Bug 202081

Summary:

Require a WebsiteDataStore when creating or resuming downloads

Product:

WebKit

Reporter:

Alex Christensen <achristensen>

Component:

New Bugs

Assignee:

Alex Christensen <achristensen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, beidson, berto, cdumez, cgarcia, ews-watchlist, ggaren, gustavo, gyuyoung.kim, rakuco, ryuan.choi, sergio, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	youennf: review+

Alex Christensen

Reported 2019-09-21 16:38:54 PDT

Move DownloadClient from WebProcessPool to WebsiteDataStore

Attachments
Patch (111.86 KB, patch) 2019-09-21 16:41 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (117.82 KB, patch) 2019-09-21 16:52 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (118.31 KB, patch) 2019-09-21 17:53 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (119.07 KB, patch) 2019-09-21 17:57 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (119.37 KB, patch) 2019-09-21 18:01 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (119.38 KB, patch) 2019-09-21 18:04 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (121.93 KB, patch) 2019-09-23 14:45 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (57.52 KB, patch) 2019-09-23 17:11 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (57.71 KB, patch) 2019-09-23 17:14 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (59.10 KB, patch) 2019-09-23 18:28 PDT, Alex Christensen	youennf: review+	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2019-09-21 16:41:13 PDT

Created attachment 379324 [details] Patch

Alex Christensen

Comment 2 2019-09-21 16:43:40 PDT

I'm assuming this will break some API that GTK already has. That API should be deprecated and replaced by one on WebsiteDataStore.

Alex Christensen

Comment 3 2019-09-21 16:52:27 PDT

Created attachment 379325 [details] Patch

EWS Watchlist

Comment 4 2019-09-21 16:53:02 PDT

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Alex Christensen

Comment 5 2019-09-21 17:53:02 PDT

Created attachment 379326 [details] Patch

Alex Christensen

Comment 6 2019-09-21 17:57:25 PDT

Created attachment 379327 [details] Patch

Alex Christensen

Comment 7 2019-09-21 18:01:37 PDT

Created attachment 379328 [details] Patch

Alex Christensen

Comment 8 2019-09-21 18:04:59 PDT

Created attachment 379329 [details] Patch

Alex Christensen

Comment 9 2019-09-23 10:53:33 PDT

iOS test failure unrelated. This is ready for review.

Alex Christensen

Comment 10 2019-09-23 13:32:51 PDT

Comment on attachment 379329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379329&action=review > Source/WebKit/UIProcess/API/C/WKContext.h:116 > +WK_EXPORT void WKContextSetDownloadClient(WKContextRef context, const WKContextDownloadClientBase* client) WK_C_API_DEPRECATED_WITH_REPLACEMENT(WKWebsiteDataStoreSetDownloadClient); Not quite ready. I need to shim WKContextSetDownloadClient onto WKWebsiteDataStoreSetDownloadClient for an internal client during the transition.

Alex Christensen

Comment 11 2019-09-23 14:45:55 PDT

Created attachment 379397 [details] Patch

Alex Christensen

Comment 12 2019-09-23 17:11:43 PDT

Created attachment 379409 [details] Patch

Alex Christensen

Comment 13 2019-09-23 17:14:20 PDT

Created attachment 379410 [details] Patch

Alex Christensen

Comment 14 2019-09-23 18:28:31 PDT

Created attachment 379414 [details] Patch

Carlos Garcia Campos

Comment 15 2019-09-24 00:57:56 PDT

Comment on attachment 379414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379414&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1661 > - return webkitWebContextGetOrCreateDownload(&context->priv->processPool->download(initiatingPage, request)); > + return webkitWebContextGetOrCreateDownload(&context->priv->processPool->download(WebKit::WebsiteDataStore::defaultDataStore().get(), initiatingPage, request)); We never use the default data store in the GLib API. Here we should use the web context data store. So this should be: return webkitWebContextGetOrCreateDownload(&context->priv->processPool->download(webkitWebsiteDataManagerGetDataStore(context->priv->websiteDataManager.get()), initiatingPage, request));

youenn fablet

Comment 16 2019-09-24 05:32:24 PDT

Comment on attachment 379414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379414&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1330 > + PAL::SessionID sessionID = dataStore.sessionID(); auto > Source/WebKit/UIProcess/WebProcessPool.cpp:1362 > + PAL::SessionID sessionID = dataStore.sessionID(); auto > Source/WebKit/UIProcess/API/C/WKContext.cpp:175 > + explicit DownloadClient(const WKContextDownloadClientBase* client, WKContextRef context) no need for explicit anymore > Source/WebKit/UIProcess/API/glib/WebKitDownloadClient.cpp:43 > + void didStart(DownloadProxy& downloadProxy) override final here and below. > Source/WebKit/UIProcess/Downloads/DownloadProxy.h:60 > + static Ref<DownloadProxy> create(DownloadProxyMap&, WebsiteDataStore&, WebProcessPool&, const WebCore::ResourceRequest&); Could take Ref<WebsiteDataStore>&&, ditto for WebProcessPool. > Source/WebKit/UIProcess/Downloads/DownloadProxy.h:103 > + explicit DownloadProxy(DownloadProxyMap&, WebsiteDataStore&, WebProcessPool&, const WebCore::ResourceRequest&); No need of explicit.

Alex Christensen

Comment 17 2019-09-24 06:18:50 PDT

(In reply to Carlos Garcia Campos from comment #15) > Comment on attachment 379414 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379414&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1661 > > - return webkitWebContextGetOrCreateDownload(&context->priv->processPool->download(initiatingPage, request)); > > + return webkitWebContextGetOrCreateDownload(&context->priv->processPool->download(WebKit::WebsiteDataStore::defaultDataStore().get(), initiatingPage, request)); > > We never use the default data store in the GLib API. Here we should use the > web context data store. So this should be: > > return > webkitWebContextGetOrCreateDownload(&context->priv->processPool- > >download(webkitWebsiteDataManagerGetDataStore(context->priv- > >websiteDataManager.get()), initiatingPage, request)); I'm about to remove the context's WebsiteDataStore.

Alex Christensen

Comment 18 2019-09-24 06:58:54 PDT

http://trac.webkit.org/r250292

Radar WebKit Bug Importer

Comment 19 2019-09-24 06:59:21 PDT

<rdar://problem/55661326>

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