[Webkit-unassigned] [Bug 26854] [GTK] Needs API to allow more control over outgoing requests
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Sep 5 12:28:38 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=26854
Xan Lopez <xan.lopez at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #39081|review? |review-
Flag| |
--- Comment #17 from Xan Lopez <xan.lopez at gmail.com> 2009-09-05 12:28:38 PDT ---
(From update of attachment 39081)
> +#include "config.h"
> +#include "ResourceResponse.h"
> +
> +#include "CString.h"
> +#include "GOwnPtr.h"
> +#include "PlatformString.h"
I'd say some of these are unused here?
> +
> +#include <libsoup/soup.h>
And this one is included in ResourceResponse.h already?
> +
> +using namespace std;
> +
> +namespace WebCore {
> +
> +SoupMessage* ResourceResponse::toSoupMessage() const
> +{
> + // This GET here is just because SoupMessage wants it, we dn't really know.
> + SoupMessage* soupMessage = soup_message_new("GET", url().string().utf8().data());
> + if (!soupMessage)
> + return 0;
> +
> + soupMessage->status_code = httpStatusCode();
> +
> + HTTPHeaderMap headers = httpHeaderFields();
> + SoupMessageHeaders* soupHeaders = soupMessage->response_headers;
> + if (!headers.isEmpty()) {
> + HTTPHeaderMap::const_iterator end = headers.end();
> + for (HTTPHeaderMap::const_iterator it = headers.begin(); it != end; ++it)
> + soup_message_headers_append(soupHeaders, it->first.string().utf8().data(), it->second.utf8().data());
> + }
> +
> + // Body data is not in the message.
> + return soupMessage;
> +}
As mentioned on IRC it would we good to share this function with
ResourceRequest, since it's identical to what's there but for one line.
> +// We convert this to string because it's easier to use strings as
> +// keys in a GHashTable.
> +static char* identifierToString(unsigned long identifier)
> +{
> + return g_strdup_printf("%ld", identifier);
> +}
> +
> void FrameLoaderClient::dispatchWillSendRequest(WebCore::DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse)
> {
> - if (redirectResponse.isNull())
> + WebKitWebView* webView = getViewFromFrame(m_frame);
> +
> + GOwnPtr<gchar> identifierString(identifierToString(identifier));
> + WebKitWebResource* webResource = webkit_web_view_get_resource(webView, identifierString.get());
> +
> + WebKitNetworkRequest* networkRequest = webkit_network_request_new_with_core_request(request);
> + WebKitNetworkResponse* networkResponse;
> +
> + // We are adding one more resource to the load, or maybe we are
> + // just redirecting a load, in which case we need to update our
> + // WebKitWebResource's knowledge of the URI.
> + if (redirectResponse.isNull()) {
> static_cast<WebKit::DocumentLoader*>(loader)->increaseLoadCount(identifier);
> +
> + networkResponse = WEBKIT_NETWORK_RESPONSE(g_object_new(WEBKIT_TYPE_NETWORK_RESPONSE, 0));
I think we should just pass NULL instead of a dummy object here? Since the
point is to inform the user of whether this is a redirect and such, I think
that makes it much more clear.
> + } else {
> + g_free(webResource->priv->uri);
> + webResource->priv->uri = g_strdup(request.url().string().utf8().data());
> +
> + networkResponse = webkit_network_response_new_with_core_response(redirectResponse);
> + }
> +
> + g_signal_emit_by_name(webView, "resource-request-starting", m_frame, webResource, networkRequest, networkResponse);
> +
> + // Feed any changes back into the ResourceRequest object.
> + request = core(networkRequest);
> + g_object_unref(networkRequest);
Also you are leaking the networkResponse object AFAICT.
> }
>
> -void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, WebCore::DocumentLoader*, const ResourceRequest&)
> +void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, WebCore::DocumentLoader*, const ResourceRequest& request)
> {
> - notImplemented();
> + WebKitWebResource* webResource = WEBKIT_WEB_RESOURCE(g_object_new(WEBKIT_TYPE_WEB_RESOURCE, 0));
> +
> + // The URI is important to have in the resource because that is what we use
> + // to fetch the WebCore object, later on. We use the private structure
> + // directly here to avoid changing the property mode from READABLE to
> + // READWRITE.
> + webResource->priv->uri = g_strdup(request.url().string().utf8().data());
I don't understand the comment. You are creating the object now, so you
certainly pass the URI you want in the parameters.
> +
> + webkit_web_view_add_resource(getViewFromFrame(m_frame), identifierToString(identifier), webResource);
> }
>
> @@ -800,13 +842,22 @@ void FrameLoaderClient::dispatchDidReceiveContentLength(WebCore::DocumentLoader*
> void FrameLoaderClient::dispatchDidFinishLoading(WebCore::DocumentLoader* loader, unsigned long identifier)
> {
> static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> + GOwnPtr<gchar> identifierString(identifierToString(identifier));
> + WebKitWebResource* webResource = webkit_web_view_get_resource(getViewFromFrame(m_frame), identifierString.get());
> +
> + const char* uri = webkit_web_resource_get_uri(webResource);
> + RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri)));
> + webkit_web_resource_init_with_core_resource(webResource, coreResource.get());
I don't understand why are you init again the resource if you are about the
remove it from the view?
> +
> + webkit_web_view_remove_resource(getViewFromFrame(m_frame), identifierString.get());
> }
>
> void FrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader* loader, unsigned long identifier, const ResourceError& error)
> {
> - // FIXME: when does this occur and what should happen?
> -
> static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> + notImplemented();
notImplemented()? The function does something. Do you know now what should it
do that it's not doing? If so add a comment.
> }
>
> + */
> +
> +#include "config.h"
> +#include "webkitnetworkresponse.h"
> +
> +#include "CString.h"
> +#include "GOwnPtr.h"
> +#include "ResourceResponse.h"
> +#include "webkitprivate.h"
> +
> +static void webkit_network_response_init(WebKitNetworkResponse* response)
> +{
> + WebKitNetworkResponsePrivate* priv = WEBKIT_NETWORK_RESPONSE_GET_PRIVATE(response);
> + response->priv = priv;
No need for the variable here.
> diff --git a/WebKit/gtk/webkit/webkitwebresource.cpp b/WebKit/gtk/webkit/webkitwebresource.cpp
> index c162578..fd4abb7 100644
> --- a/WebKit/gtk/webkit/webkitwebresource.cpp
> +++ b/WebKit/gtk/webkit/webkitwebresource.cpp
> @@ -27,6 +27,8 @@
> #include "KURL.h"
> #include "PlatformString.h"
> #include "SharedBuffer.h"
> +#include "webkitenumtypes.h"
> +#include "webkitmarshal.h"
> #include "wtf/Assertions.h"
>
> #include <glib.h>
> @@ -50,13 +52,34 @@ enum {
> PROP_URI,
> PROP_MIME_TYPE,
> PROP_ENCODING,
> - PROP_FRAME_NAME
> + PROP_FRAME_NAME,
> };
Uh? :)
> +void webkit_web_resource_init_with_core_resource(WebKitWebResource* webResource, PassRefPtr<ArchiveResource> resource)
> +{
> + WebKitWebResourcePrivate* priv = webResource->priv;
> +
> + if (priv->resource)
> + priv->resource->deref();
> +
> + webkit_web_resource_cleanup(webResource);
> +
> + priv->resource = resource.releaseRef();
> +}
Since this function is only used before removing a resource now I'm really
curious about what's its point.
> +
> /**
> * webkit_web_resource_new:
> * @data: the data to initialize the #WebKitWebResource
> @@ -270,11 +298,14 @@ G_CONST_RETURN gchar* webkit_web_resource_get_uri(WebKitWebResource* webResource
> g_return_val_if_fail(WEBKIT_IS_WEB_RESOURCE(webResource), NULL);
>
> WebKitWebResourcePrivate* priv = webResource->priv;
> +
> + if (priv->uri)
> + return priv->uri;
> +
> if (!priv->resource)
> return NULL;
>
> - if (!priv->uri)
> - priv->uri = g_strdup(priv->resource->url().string().utf8().data());
> + priv->uri = g_strdup(priv->resource->url().string().utf8().data());
>
> return priv->uri;
This seems subtle enough to warrant a comment. Was it broken before?
> }
> @@ -344,3 +375,4 @@ G_CONST_RETURN gchar* webkit_web_resource_get_frame_name(WebKitWebResource* webR
>
> return priv->frameName;
> }
> +
> + /**
> + * WebKitWebView::resource-request-starting:
> + * @web_view: the object which received the signal
> + * @web_frame: the #WebKitWebFrame whose load dispatched this request
> + * @web_resource: an empty #WebKitWebResource object
> + * @request: the #WebKitNetworkRequest that will be dispatched
> + * @response: the #WebKitNetworkResponse representing the redirect
> + * response, if any
> + *
> + * Emitted when a request is about to be sent. You can modify the
> + * request, by adding/removing/replacing headers, or changing the
> + * URI, using the #SoupMessage object it carries, if it is
> + * present. See webkit_network_request_get_message(). Setting the
> + * request URI to "about:blank" will effectively cause the request
> + * to load nothing, and can be used to disable the loading of
> + * specific resources.
> + *
> + * Notice that information about an eventual redirect are
> + * available in @response's #SoupMessage, not in the #SoupMessage
> + * carried by the @request. If @response doesn't have a
> + * #SoupMessage (i.e. if webkit_network_response_get_message()
> + * returns %NULL), then this is not a redirected request.
As mentioned, I think it's better to either have NULL or a full response.
> + *
> + * The #WebKitWebResource object will be the same throughout all
> + * the lifetime of the resource, but the contents may change from
> + * inbetween signal emissions.
> + *
> + * Since: 1.1.14
> + */
> + webkit_web_view_signals[RESOURCE_REQUEST_STARTING] = g_signal_new("resource-request-starting",
> + G_TYPE_FROM_CLASS(webViewClass),
> + (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> + 0,
> + NULL, NULL,
> + webkit_marshal_VOID__OBJECT_OBJECT_OBJECT_OBJECT,
> + G_TYPE_NONE, 4,
> + WEBKIT_TYPE_WEB_FRAME,
> + WEBKIT_TYPE_WEB_RESOURCE,
> + WEBKIT_TYPE_NETWORK_REQUEST,
> + WEBKIT_TYPE_NETWORK_RESPONSE);
> +
> +
> diff --git a/WebKit/gtk/webkitmarshal.list b/WebKit/gtk/webkitmarshal.list
> index aa0d40c..cff8018 100644
> --- a/WebKit/gtk/webkitmarshal.list
> +++ b/WebKit/gtk/webkitmarshal.list
> @@ -13,7 +13,11 @@ OBJECT:OBJECT
> OBJECT:STRING,STRING,POINTER
> OBJECT:VOID
> VOID:OBJECT,OBJECT
> +VOID:OBJECT,OBJECT,OBJECT,OBJECT
> +VOID:OBJECT,POINTER
> VOID:OBJECT,POINTER,POINTER
> VOID:OBJECT,STRING
> +VOID:OBJECT,ULONG
> VOID:STRING
> VOID:STRING,STRING
> +VOID:STRING,POINTER
I don't think you need all these now, just the first one.
> --
> 1.6.3.3
>
Marking as r- since I think there's some things to clarify/polish still, but I
think the new APIs are good.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list