Bug 206037 - Expand _WKResourceLoadDelegate callbacks
Summary: Expand _WKResourceLoadDelegate callbacks
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: 2020-01-09 14:38 PST by Alex Christensen
Modified: 2020-01-10 14:38 PST (History)
4 users (show)

See Also:


Attachments
Patch (73.12 KB, patch)
2020-01-09 14:58 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (72.64 KB, patch)
2020-01-09 15:08 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (72.64 KB, patch)
2020-01-09 15:53 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (77.90 KB, patch)
2020-01-10 09:48 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-01-09 14:38:37 PST
Expand _WKResourceLoadDelegate callbacks
Comment 1 Alex Christensen 2020-01-09 14:58:52 PST
Created attachment 387277 [details]
Patch
Comment 2 Alex Christensen 2020-01-09 15:08:10 PST
Created attachment 387280 [details]
Patch
Comment 3 Alex Christensen 2020-01-09 15:53:34 PST
Created attachment 387284 [details]
Patch
Comment 4 youenn fablet 2020-01-10 09:08:18 PST
Comment on attachment 387284 [details]
Patch

Seems fine but I have some questions.

This API is read-only.
Do we also want to modify/cancel all these requests?
Do we envision replacing InjectedBundle loading API with this API?

Do we want to see these requests before or after service worker?
Currently, this is after service worker.

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:104
> +    return nextIdentifier++;

Can we use an ObjectIdentifier?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:111
> +    void didReceiveChallenge(const WebCore::AuthenticationChallenge&) override;

final

> Source/WebKit/UIProcess/API/APIResourceLoadInfo.h:40
> +    ResourceLoadInfo(WebKit::ResourceLoadInfo&& info)

explicit and private.
Comment 5 Alex Christensen 2020-01-10 09:11:31 PST
(In reply to youenn fablet from comment #4)
> Comment on attachment 387284 [details]
> Patch
> 
> Seems fine but I have some questions.
> 
> This API is read-only.
> Do we also want to modify/cancel all these requests?
That is not planned right now.  Hopefully never.
> Do we envision replacing InjectedBundle loading API with this API?
The InjectedBundle has the ability to mutate requests before sending.  This is not a replacement for that ability.
> 
> Do we want to see these requests before or after service worker?
> Currently, this is after service worker.
After.  This records what is actually sent on the network.  If we find we need service worker requests, too, we can add that.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387284&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:104
> > +    return nextIdentifier++;
> 
> Can we use an ObjectIdentifier?
No, because NetworkResourceLoader already has an identifier() method that returns the identifier according to the web process, but this identifier needs to be unique to the network process.
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.h:111
> > +    void didReceiveChallenge(const WebCore::AuthenticationChallenge&) override;
> 
> final
> 
> > Source/WebKit/UIProcess/API/APIResourceLoadInfo.h:40
> > +    ResourceLoadInfo(WebKit::ResourceLoadInfo&& info)
> 
> explicit and private.
Will do before committing.
Comment 6 youenn fablet 2020-01-10 09:17:15 PST
> > Can we use an ObjectIdentifier?
> No, because NetworkResourceLoader already has an identifier() method that
> returns the identifier according to the web process, but this identifier
> needs to be unique to the network process.

I mean, can we use an ObjectIdentifier<NetworkResourceLoadIdentifierType> instead of an integer.
Comment 7 Alex Christensen 2020-01-10 09:48:18 PST
Created attachment 387350 [details]
Patch
Comment 8 Alex Christensen 2020-01-10 09:53:18 PST
(In reply to youenn fablet from comment #6)
> I mean, can we use an ObjectIdentifier<NetworkResourceLoadIdentifierType>
> instead of an integer.
That worked, but it made it so my identifiers were not 1, 2, 3... but rather numbers with all ObjectIdentifiers mixed in.  That's probably fine, but I had to update the test too.  I shouldn't be relying on deterministic identifiers anyways.
Comment 9 Alex Christensen 2020-01-10 10:06:56 PST
http://trac.webkit.org/r254345
Comment 10 Radar WebKit Bug Importer 2020-01-10 10:07:16 PST
<rdar://problem/58482281>
Comment 11 Jonathan Bedard 2020-01-10 13:40:34 PST
New test has been crashing since it was committed:
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.ResourceLoadDelegate.BeaconAndSyncXHR
Comment 12 Alex Christensen 2020-01-10 13:41:22 PST
Looking into it...
Comment 13 Alex Christensen 2020-01-10 14:38:46 PST
http://trac.webkit.org/r254367 should fix those tests.