[webkit-reviews] review granted: [Bug 107974] HTTP Authentication should be directly between the NetworkProcess and the UIProcess : [Attachment 184793] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 107974: HTTP Authentication should be directly between the NetworkProcess
and the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=107974

Attachment 184793: Patch v1
https://bugs.webkit.org/attachment.cgi?id=184793&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184793&action=review


We can m_identifier from AuthenticationChallengeBase now. Will you do that in a
follow-up patch, or should I?

> Source/WebKit2/ChangeLog:29
> +	   Include the WebPage and WebFrame ID for the originator of this
request in case it results in a challenge:

These IDs are generated by UI process, correct?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:235
> +    uint64_t webPageID = loadParameters().webPageID();
> +    uint64_t webFrameID = loadParameters().webFrameID();

We no longer copy the whole IPC parameters structure into SchedulableLoader,
this is why the build failed.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:71
> +uint64_t AuthenticationManager::mapChallengeToIdentifier(const
WebCore::AuthenticationChallenge& authenticationChallenge)

Perhaps "establishIdentifierForChallenge"?

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:73
> +    uint64_t challengeID = generateAuthenticationChallengeID();

That function is not thread safe. Switch to atomicIncrement?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:83
> +    DEFINE_STATIC_LOCAL(WebProcessProxy::WebPageProxyMap, pageMap, ());
> +    return pageMap;

Maybe ASSERT(isMainThread())?

> Source/WebKit2/UIProcess/WebProcessProxy.h:82
> -    WebPageProxy* webPage(uint64_t pageID) const;
> +    static WebPageProxy* webPage(uint64_t pageID);

I don't see any issue with this, but suggest double-checking with Sam or
Anders.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:231
> +    NetworkResourceLoadParameters loadParameters(resourceLoadIdentifier, 0,
0, request, ResourceLoadPriorityHighest, SniffContent, storedCredentials,
context->storageSession().isPrivateBrowsingSession());

This is worth a ChangeLog comment. Implementing authentication for sync
requests needs to start at ResourceHandleMac level, but this wasn't immediately
obvious to me.