Bug 132308 – Shortcircuit shouldUseCredentialStorage callback

• Component: Platform
• Status: RESOLVED FIXED

[Pratik Solanki]

If we are going to tell CFNetwork to use the credential storage then we don't really need a callback from them. Default answer is true.

[Pratik Solanki]

Created attachment 230330 [details]
Patch

[Brady Eidson]

Comment on attachment 230330 [details]
Patch

Why is this important for ResourceHandleCF but not ResourceHandleMac?

[Pratik Solanki]

(In reply to comment #2)
> (From update of attachment 230330 [details])
> Why is this important for ResourceHandleCF but not ResourceHandleMac?

How would we do this for ResourceHandleMac? How do we dynamically remove the objc method -[WebCoreResourceHandleAsOperationQueueDelegate connectionShouldUseCredentialStorage:]? We should do it for ResourceHandleMac if it is possible.

[Pratik Solanki]

Comment on attachment 230330 [details]
Patch

Marking for review again unless someone has an idea for how to do this for ResourceHandleMac.

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 230330 [details] [details])
> > Why is this important for ResourceHandleCF but not ResourceHandleMac?
> 
> How would we do this for ResourceHandleMac? How do we dynamically remove the objc method -[WebCoreResourceHandleAsOperationQueueDelegate connectionShouldUseCredentialStorage:]? We should do it for ResourceHandleMac if it is possible.

You don't dynamically *remove* the objective-C method.

You either dynamically *add* the method (which is possible, if not a little dirty), or you have a base-class delegate without the method, then subclass to add the method, and choose which class to instantiate.

[Brady Eidson]

Comment on attachment 230330 [details]
Patch

I listed two ways this could be done for Mac in a comment.

[Pratik Solanki]

Created attachment 230653 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 230653 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Shortcircuit shouldUseCredentialStorage callback
> +        https://bugs.webkit.org/show_bug.cgi?id=132308

This looks like a good idea to me.

The code is very confusing, so I think that this change should balance that by cleaning up at least some of the cruft right away. In particular, shouldn't shouldUseCredentialStorageAsync be eliminated? As we hardcode the answer (in NetworkProcess) by either providing or not providing a callback, all this complicated machinery is essentially dead code.

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:338
> +- (BOOL)connectionShouldUseCredentialStorage:(NSURLConnection *)connection

This function looks like it should always return NO (or YES? I'm confused).

Is there a reason to still jump to the main thread?

[Pratik Solanki]

Comment on attachment 230653 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=132308
> 
> This looks like a good idea to me.
> 
> The code is very confusing, so I think that this change should balance that by cleaning up at least some of the cruft right away. In particular, shouldn't shouldUseCredentialStorageAsync be eliminated? As we hardcode the answer (in NetworkProcess) by either providing or not providing a callback, all this complicated machinery is essentially dead code.

That's true. I realized that and was going to do in a  followup path but maybe I can just clean this up now. Will definitely be easier.

>> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:338
>> +- (BOOL)connectionShouldUseCredentialStorage:(NSURLConnection *)connection
> 
> This function looks like it should always return NO (or YES? I'm confused).
> 
> Is there a reason to still jump to the main thread?

Yeah, we don't need to jump to main thread. We can just return NO.

[Pratik Solanki]

Created attachment 230694 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 230694 [details]
Patch

r=me

Please unbreak the builds before landing.

[Pratik Solanki]

Created attachment 230723 [details]
For the bots

Updated patch for the bots. I also removed continueShouldUseCredentialStorage() with this version.

[Pratik Solanki]

<rdar://problem/16806708>

[Pratik Solanki]

Committed r168232: <http://trac.webkit.org/changeset/168232>

[Andreas Kling]

Mavericks WK2 Debug is crashing on a couple of tests after this change:
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/4408

Could you take a look, or should we roll it out?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132543

[Pratik Solanki]

Committed r168248: <http://trac.webkit.org/changeset/168248>