Bug 99351

Summary: [GTK][Soup] Implement the default authentication dialog via WebCoreSupport
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, danw, gustavo, japhet, philn, rakuco, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99352, 99913, 99914    
Attachments:
Description Flags
Patch
none
Patch
none
Patch cgarcia: review+

Description Martin Robinson 2012-10-15 12:30:50 PDT
Instead of using a SoupSession feature to enable the default authentication dialog, we should use WebCoreSupport. This is the first step toward full-class support for authentication.
Comment 1 Martin Robinson 2012-10-17 11:25:40 PDT
Created attachment 169222 [details]
Patch
Comment 2 Martin Robinson 2012-10-17 11:26:07 PDT
The reason the changes in the Soup backend are GTK+ only is that the EFL port still uses a SoupFeature to implement the authentication dialog.
Comment 3 Martin Robinson 2012-10-17 12:57:57 PDT
Created attachment 169238 [details]
Patch
Comment 4 Carlos Garcia Campos 2012-10-22 05:17:08 PDT
Comment on attachment 169238 [details]
Patch

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

This is great, I'm happy to see progress on this, thanks!. My only concern is the changes to the default session, because I think they might break EFL port in WebKit1. In any case, since those changes look unrelated to the patch, maybe you can split the patch and I would review the authentication specific changes.

> Source/WebCore/ChangeLog:38
> +        (WebCore::ResourceHandleInternal::soupSession): Use the default SoupSession instead of the
> +        one from the context -- the context is always storing the default one anyway.

This is true in GTK+ port and WebKit2, but not in EFL, I think. Their implementation of the FrameNetworkingContext returns the SoupSession associated to the web view, and they have API to set a SoupSession to the a web view. I'll add EFL guys to the CC to confirm it.

> Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:48
> +    ProtectionSpaceAuthenticationScheme scheme = ProtectionSpaceAuthenticationSchemeDefault;

This will never happen, since there's a final else in the if, or is it to silence the compiler?

> Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:60
> +    SoupURI* soupURI = soup_message_get_uri(message);

The message is only passed to get the uri, would it make sense to pass the SoupURI instead to the function? I guess it's more convenient to pass the message as it's what the consructor receives.

> Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:69
> +        retrying ? 0 : 1,

retrying ? 0 : 1, // previousFailureCount ?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:268
>  SoupSession* ResourceHandleInternal::soupSession()
>  {
> -    SoupSession* session = sessionFromContext(m_context.get());
> +    SoupSession* session = ResourceHandle::defaultSession();
>      ensureSessionIsInitialized(session);
>      return session;
>  }

If we are going to use always the default session, maybe we can just remove this, or ::defaultSession() since both return the same session in the end.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1043
> +        g_signal_connect(G_OBJECT(session), "authenticate", G_CALLBACK(authenicateCallback), 0);

g_signal_connect expects a gpointer not a GObject, so we don't need the cast.
Comment 5 Carlos Garcia Campos 2012-10-22 05:19:16 PDT
See https://bugs.webkit.org/show_bug.cgi?id=77341
Comment 6 Martin Robinson 2012-10-22 10:33:32 PDT
Created attachment 169936 [details]
Patch
Comment 7 Martin Robinson 2012-10-22 10:36:58 PDT
(In reply to comment #4)
> (From update of attachment 169238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169238&action=review
> 
> This is great, I'm happy to see progress on this, thanks!. My only concern is the changes to the default session, because I think they might break EFL port in WebKit1. In any case, since those changes look unrelated to the patch, maybe you can split the patch and I would review the authentication specific changes.

Thanks for the review. I've removed the changes that don't account for non-default sessions.

> > Source/WebCore/ChangeLog:38
> > +        (WebCore::ResourceHandleInternal::soupSession): Use the default SoupSession instead of the
> > +        one from the context -- the context is always storing the default one anyway.
> 
> This is true in GTK+ port and WebKit2, but not in EFL, I think. Their implementation of the FrameNetworkingContext returns the SoupSession associated to the web view, and they have API to set a SoupSession to the a web view. I'll add EFL guys to the CC to confirm it.

Okay. I've tried to remove all the changes that make this assumption in my patch.

> 
> > Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:48
> > +    ProtectionSpaceAuthenticationScheme scheme = ProtectionSpaceAuthenticationSchemeDefault;
> 
> This will never happen, since there's a final else in the if, or is it to silence the compiler?

I have a instinct never to declare a variable without assigning it a default value, but I don't mind changing it in this case.

> 
> > Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:60
> > +    SoupURI* soupURI = soup_message_get_uri(message);
> 
> The message is only passed to get the uri, would it make sense to pass the SoupURI instead to the function? I guess it's more convenient to pass the message as it's what the consructor receives.

Yeah, I think this is slightly simpler.


> > Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:69
> > +        retrying ? 0 : 1,
> 
> retrying ? 0 : 1, // previousFailureCount ?

Okay.
 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1043
> > +        g_signal_connect(G_OBJECT(session), "authenticate", G_CALLBACK(authenicateCallback), 0);
> 
> g_signal_connect expects a gpointer not a GObject, so we don't need the cast.

Okay. I've fixed the other one too.
Comment 8 Carlos Garcia Campos 2012-10-23 09:00:29 PDT
Comment on attachment 169936 [details]
Patch

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

Excellent!

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:672
> -static void setSoupRequestInitiaingPageID(SoupRequest* request, uint64_t initiatingPageID)
> +static void setSoupRequestInitiaingPageID(SoupRequest* request, NetworkingContext* context)

The name looks a bit weird now that it doesn't receive a page id, maybe setSoupRequestInitiaingPageIDFromContext or something similar.
Comment 9 Martin Robinson 2012-10-23 18:08:55 PDT
Committed r132286: <http://trac.webkit.org/changeset/132286>