Bug 154939 – Use CredentialStorage with NetworkSession

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use CredentialStorage with NetworkSession

[Alex Christensen]

Created attachment 272704 [details]
Patch

[Alex Christensen]

Comment on attachment 272704 [details]
Patch

First patch is (obviously) WIP

[Alex Christensen]

Created attachment 272711 [details]
Patch

[Alex Christensen]

Created attachment 272712 [details]
Patch

[Alex Christensen]

Created attachment 272746 [details]
Patch

[Darin Adler]

Comment on attachment 272746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272746&action=review

> Source/WebCore/platform/network/CredentialStorage.h:41
> +class WEBCORE_EXPORT CredentialStorage {

I’ve never seen us use this before. Is this a good pattern? Better than exporting individual functions?

> Source/WebKit2/config.h:80
> +// FIXME: We should work towards not using CredentialStorage in WebKit2.

Confusing comment. Would be less confusing if it said why.

Also confusing because compile-time flags don’t affect just WebKit2. They affect compilation of headers from JavaScriptCore and WebCore as well. So it’s not like we can set these flags completely independently for WebCore and WebKit2. It’s dangerous to start thinking of these flags as framework-specific, when they actually need to be set consistently for the entire suite of WebKit frameworks given how we use them in headers.

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:93
> +            ASSERT_NOT_REACHED();

What would we want to do in a release build? Is doing nothing here really OK?

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:147
> +        completionHandler(AuthenticationChallengeDisposition::UseCredential, WebCore::Credential());

I think { } is nicer than WebCore::Credential().

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:309
> +            ASSERT_NOT_REACHED();

Same question about whether this is really OK in release builds.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:133
> +                WebCore::Credential webCredential(credential.user(), credential.password(), WebCore::CredentialPersistenceNone);

Name here is unclear. We already have a local variable named "credential" of the same type. I don’t think that webCredential makes it clear what is difference about this second credential.

[Alex Christensen]

(In reply to comment #6)
> Comment on attachment 272746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272746&action=review
> 
> > Source/WebCore/platform/network/CredentialStorage.h:41
> > +class WEBCORE_EXPORT CredentialStorage {
> 
> I’ve never seen us use this before. Is this a good pattern? Better than
> exporting individual functions?
This exports everything in the class.  This is necessary for exporting the vtable, but in this case I'm just being lazy.  I'll add export macros for exactly what I need.
> 
> > Source/WebKit2/config.h:80
> > +// FIXME: We should work towards not using CredentialStorage in WebKit2.
> 
> Confusing comment. Would be less confusing if it said why.
> 
> Also confusing because compile-time flags don’t affect just WebKit2. They
> affect compilation of headers from JavaScriptCore and WebCore as well. So
> it’s not like we can set these flags completely independently for WebCore
> and WebKit2. It’s dangerous to start thinking of these flags as
> framework-specific, when they actually need to be set consistently for the
> entire suite of WebKit frameworks given how we use them in headers.
This, like USE_NETWORK_SESSION, is only for WebKit2.  I'll rename it to USE_CREDENTIAL_STORAGE_WITH_NETWORK_SESSION to be more clear.
> 
> > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:93
> > +            ASSERT_NOT_REACHED();
> 
> What would we want to do in a release build? Is doing nothing here really OK?
I'll make it a RELEASE_ASSERT_NOT_REACHED.  This is one of those assertions that should never be hit because if we are doing any loading with a session, that means the session exists.
> Name here is unclear. We already have a local variable named "credential" of
> the same type. I don’t think that webCredential makes it clear what is
> difference about this second credential.
Will rename.

[Alex Christensen]

Created attachment 272771 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272771 [details]
Patch

Rejecting attachment 272771 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
es/Data/EWS/WebKit/Source/WebKit2/Shared/ChildProcess.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/ChildProcess.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/AuthenticationManager.mac.o Shared/Authentication/mac/AuthenticationManager.mac.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/917246

[Alex Christensen]

Created attachment 272784 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/197526