Bug 99351

Created attachment 169222 [details] Patch

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.

Created attachment 169238 [details] Patch

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.

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

Created attachment 169936 [details] Patch

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

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