Bug 99351 – [GTK][Soup] Implement the default authentication dialog via WebCoreSupport

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 169222 [details]
Patch

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 169238 [details]
Patch

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

See https://bugs.webkit.org/show_bug.cgi?id=77341

[Martin Robinson]

Created attachment 169936 [details]
Patch

[Martin Robinson]

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

[Carlos Garcia Campos]

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.

[Martin Robinson]

Committed r132286: <http://trac.webkit.org/changeset/132286>