Summary: | Coalesce authentication credential requests | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, barraclough, beidson, cdumez, commit-queue, darin, dbates, ddkilzer, mjs, webkit-bug-importer | ||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Daniel Bates
2014-01-31 12:01:43 PST
This is usually not an issue because main resource tends to be authenticated, and subsequent loads don't start until it loads. But it can get really annoying for something like the dashboard. Also, reopening windows after browser restart can result in multiple requests for the same credentials, if you had the same site open in multiple windows. (In reply to comment #1) > This is usually not an issue because main resource tends to be authenticated, and subsequent loads don't start until it loads. > > But it can get really annoying for something like the dashboard. Also, reopening windows after browser restart can result in multiple requests for the same credentials, if you had the same site open in multiple windows. This behavior drives me crazy every time I restart Safari. *** Bug 147496 has been marked as a duplicate of this bug. *** Created attachment 257965 [details]
Patch
Comment on attachment 257965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257965&action=review > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:58 > + // Do not coalesce server trust evaluation requests. This comment either needs to explain why, or to be dropped. > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:73 > +uint64_t AuthenticationManager::addChallengeToHashMap(const WebCore::AuthenticationChallenge& authenticationChallenge) I'd say addChallengeToChallengeMap. Not a great name, but spelling out the data type in "addChallengeToHashMap" seems even less useful. > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:95 > +Vector<uint64_t> AuthenticationManager::coalesceChallenges(uint64_t challengeID) const This took me awhile to understand - why does a "coalesce" function take a single challenge, what can it coalesce it with? How would you feel about one of these ideas: - coalesceChallengesMatching(uint64_t challengeID) - or coalesceChallenges(const ProtectionSpace&) Comment on attachment 257965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257965&action=review >> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:58 >> + // Do not coalesce server trust evaluation requests. > > This comment either needs to explain why, or to be dropped. Proposing: "Do not coalesce server trust evaluation requests because ProtectionSpace comparison does not evaluate server trust (e.g. certificate)." >> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:95 >> +Vector<uint64_t> AuthenticationManager::coalesceChallenges(uint64_t challengeID) const > > This took me awhile to understand - why does a "coalesce" function take a single challenge, what can it coalesce it with? > > How would you feel about one of these ideas: > - coalesceChallengesMatching(uint64_t challengeID) > - or coalesceChallenges(const ProtectionSpace&) I'd prefer the first one. The second one is clear but a bit annoying because we then have to move the HashMap lookup to every call site. Created attachment 257977 [details]
Patch
Comment on attachment 257977 [details] Patch Clearing flags on attachment: 257977 Committed r187691: <http://trac.webkit.org/changeset/187691> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 258586 [details]
Patch
Uploaded a follow-up patch to fix post-mortem review comments from Darin on Bug 147496. Comment on attachment 258586 [details] Patch Clearing flags on attachment: 258586 Committed r188198: <http://trac.webkit.org/changeset/188198> All reviewed patches have been landed. Closing bug. |