[webkit-reviews] review granted: [Bug 131349] [WK2] Authentication dialog is displayed for cross-origin XHR : [Attachment 237847] Precomputing credential request policy in WebProcess

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 131349: [WK2] Authentication dialog is displayed for cross-origin XHR
https://bugs.webkit.org/show_bug.cgi?id=131349

Attachment 237847: Precomputing credential request policy in WebProcess
https://bugs.webkit.org/attachment.cgi?id=237847&action=review

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


Looks good to me. Please address the comments before landing.

> Source/WebCore/loader/ResourceLoader.cpp:541
> +bool ResourceLoader::canAskUserForCredentials() const

isAllowedToAskClientForCredentials would be a more accurate name:

1. "can" is ambiguous, one could misunderstand it and decide that we are
talking about ability, not about permission.
2. It's about asking the client, which may or may not present UI to the user
(e.g. WebKitTestRunner does not).

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:388
> +    // The Web Process is precomputing client credential policy, hence below
ASSERT.

I'd say:

// NetworkResourceLoader does not know whether the request is cross origin, so
Web process computes an applicable credential policy for it.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:92
> +    // TODO: check whether we need to update NetworkResourceLoader
clientCredentialPolicy in case loader policy is
DoNotAskClientForCrossOriginCredentials.

s/TODO/FIXME/

I'd make an actual question out of it:

// FIXME: Do we need to update NetworkResourceLoader clientCredentialPolicy in
case loader policy is DoNotAskClientForCrossOriginCredentials?