Bug 132308

Created attachment 230330 [details] Patch

Comment on attachment 230330 [details] Patch Why is this important for ResourceHandleCF but not ResourceHandleMac?

(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.

Comment on attachment 230330 [details] Patch Marking for review again unless someone has an idea for how to do this for ResourceHandleMac.

(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.

Comment on attachment 230330 [details] Patch I listed two ways this could be done for Mac in a comment.

Created attachment 230653 [details] Patch

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?

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.

Created attachment 230694 [details] Patch

Comment on attachment 230694 [details] Patch r=me Please unbreak the builds before landing.

Created attachment 230723 [details] For the bots Updated patch for the bots. I also removed continueShouldUseCredentialStorage() with this version.

<rdar://problem/16806708>

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

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?

Re-opened since this is blocked by bug 132543

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