Bug 206037

Created attachment 387277 [details]
Patch

Created attachment 387280 [details]
Patch

Created attachment 387284 [details]
Patch

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.

(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.

> > 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.

Created attachment 387350 [details]
Patch

(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.

http://trac.webkit.org/r254345

<rdar://problem/58482281>

New test has been crashing since it was committed:
https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.ResourceLoadDelegate.BeaconAndSyncXHR

Looking into it...

http://trac.webkit.org/r254367 should fix those tests.

http://trac.webkit.org/r256444