[webkit-reviews] review denied: [Bug 29160] Safari 4 unable to change settings on Linksys Routers : [Attachment 39469] Updated fix + DRT enhancements + Layout Test

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 29160: Safari 4 unable to change settings on Linksys Routers
https://bugs.webkit.org/show_bug.cgi?id=29160

Attachment 39469: Updated fix + DRT enhancements + Layout Test
https://bugs.webkit.org/attachment.cgi?id=39469&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
This only changes the behavior for Basic authentication scheme. I think that's
worth mentioning in ChangeLog.

+static OriginToDefaultCredentialMap& originToDefaultCredentialMap()

Maybe it's also worth adding "Basic" somewhere in this name, or in a comment
next to it.

+#include "Credential.h"
+#include "CString.h"

This is not properly sorted, we use case-sensitive sort for headers.

 #include "CookieStorageWin.h"
+#include "CredentialStorage.h"
 #include "CString.h"

Same issue (pre-existing).

+	     // This case should never happen, as all URL paths should start
with a leading /

All _HTTP_ paths? There are protocolInHTTPFamily() checks at call sites, seems
worth adding an assertion in the function itself.

+	 // <rdar://problem/7174050> - For URLs that match the paths of those
previously challenged for HTTP Basic authentication, 
+	 // we need to try and reuse the credential preemptively, as allowed by
RFC 2617.

Two times in a row, I read this as a FIXME comment. Maybe it's just me, or
maybe it needs to be more assertive than "we need to" :)

+	 CString encoded = unencoded.utf8().base64Encode();

We should ideally check if it's UTF-8 is what other browsers use - it may also
be page encoding. But I'm not sure if credentials encoding is something we do
correctly now.

     if (!challenge.previousFailureCount() && (!client() ||
client()->shouldUseCredentialStorage(this))) {
-	 NSURLCredential *credential =
WebCoreCredentialStorage::get([mac(challenge) protectionSpace]);
-	 if (credential) {
-	     [challenge.sender() useCredential:credential
forAuthenticationChallenge:mac(challenge)];
+	 Credential credential =
CredentialStorage::get(challenge.protectionSpace());
+	 if (!credential.isEmpty()) {
+	     [challenge.sender() useCredential:mac(credential)
forAuthenticationChallenge:mac(challenge)];
	     return;
	 }

If default credentials are incorrect, are we going to send them again on 401,
and only then ask the user?

A separate question - aren't we going to store proxy credentials as default
credentials for an URL after handling a 407 response?

This patch looks really good to me, both code changes and the fact that it
greatly improves DRT capabilities. I have to say r- since 407 looks like an
important issue, even though I'm not 100% sure that it's real.