|
Description
Gustavo Noronha (kov)
2008-04-18 18:20:41 PDT
Created attachment 20681 [details]
draft patch implementing method and http_headers properties
My first drafts, I'm attaching so that anybody interested can comment and guide my work on this.
Created attachment 20882 [details]
new draft
I've moved a bit forward (attended FISL and had to catch up on work, so not a lot to show =D). I wrote some code for GtkLauncher that makes use of the WebKitNetworkRequest API, to understand it better and to serve as a test platform. I would be very grateful for comments on the directions I'm taking.
I'll start working on documenting what I have already written, and then finish implementing what is left.
Created attachment 20895 [details]
almost complete implementation of a gobject exposing ResourceRequest
I think the patch is now mostly complete, if the purpose is exposing information from ResourceRequest only. The area that does need improvement is provind a nicely parsed form_data thingy instead of only providing a gchar* for the body.
The dialog in GtkLauncher exercises all of the getter methods, and serves as a nice way of testing/debugging the code.
I am still wondering whether most of those should not be real properties, though, and if the changes to the WebKitNetworkRequest object are to be propagated to the original ResourceRequest object or not.
gtk-doc-style documentation is still missing, as well, but I will work on it tomorrow.
Comment on attachment 20895 [details]
almost complete implementation of a gobject exposing ResourceRequest
setting the review flag correctly
Created attachment 20999 [details]
Updated implementation, with some more goodies
Messing around with the code answered some of my doubts. By looking at FrameLoaderClientGtk's needs, and webkitwebframe.cpp I understand that the original ResourceRequest which I'm mapping needs to be carried with WebKitNetworkRequest, and updated with the values. Currently I'm carrying a copy, though, and I update the original where it needs to be updated, such as in the dispatchWillSendRequest delegate.
Some rough edges still need thought and work; a more memory efficient way of handling the internal hash table which contains the headers, only creating it when really needed, for instance.
Also, we still have no implementation for ResourceResponse as WebKitNetworkResponse, so the new signal I created for webkitwebview, request-started is missing the redirectResponse argument it should have. I will write a placeholder to fix this, or perhaps even implemente WebKitNetworkResponse completely for this patch to land, if you prefer it that way.
Sounds about right?
Created attachment 21070 [details]
proposed patch
ok, so this is my proposal for landing WebKitNetworkRequest (along with an initial WebKitNetworkResponse); comments? =)
I haven't looked at the patch, so perhaps I'm preaching to the choir here... but having object structs directly accessible is considered a very bad thing (also note the GTK+ 3.0 discussion!). Accessor methods are much more versatile. Please discard my last comments, the patch contains both webkit_network_request_[gs]et_uri() functions. (In reply to comment #8) > Please discard my last comments, the patch contains both > webkit_network_request_[gs]et_uri() functions. Yeah, one of the main goals I tried to keep in mind while writing that was to only make public what should be public. I am not even sure accessor functions would be required, since we have g_object_[gs]et, but since the other parts of the public API have them, I decided to follow. (In reply to comment #6) > ok, so this is my proposal for landing WebKitNetworkRequest (along with an > initial WebKitNetworkResponse); comments? =) I would omit request-redirected/request-started for now and add it when we have both this patch and the one to clean the frame loader signals, see bug #17066. Moreover, having two signals means that people will forget to connect to both, a single signal is better IMHO. Suggestions for the name?. Another problem is that we don't pass the object indentified by the identifier argument. >+ ResourceResponse responseCopy = resourceResponse; >+ WebKitNetworkResponse* response; >+ >+ response = webkit_network_response_new_from_resource_response(responseCopy); Initialisation should be on the same line as declaration. >struct _WebKitNetworkRequestPrivate { >+ ResourceRequest request; >+ > gchar* uri; >+ WebKitReqCachePolicy cache_policy; >+ gdouble timeout_interval; >+ gchar* main_document_uri; >+ gchar* http_method; >+ GHashTable *http_headers; >+ gboolean allow_http_cookies; >+ gchar *body; >+ gsize body_size; > }; You should always use the WebKit style for things that are not exposed in the public API. Why are you keeping in WebKitNetworkRequestPrivate both the ResourceRequest and the values (like cache_policy) that can be easily extracted from it? Maybe it makes sense for strings to avoid conversions and duplications but not for just integers. >+static WebKitReqCachePolicy cache_policy_from_resource_request(const ResourceRequest& resourceRequest) >+{ >+ ResourceRequestCachePolicy cache_policy = resourceRequest.cachePolicy(); >+ >+ switch(cache_policy) >+ { This could just be something like: static WebKitReqCachePolicy kit(ResourceRequestCachePolicy cachePolicy) { return (WebKitReqCachePolicy)cachePolicy; } Then add a comment to the public header to keep the enums in sync. >+void webkit_network_request_set_cache_policy(WebKitNetworkRequest* request, const WebKitReqCachePolicy cache_policy) >+{ Ditto, just define a core() function that casts the enum. >+ priv->http_headers = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free); >+ HTTPHeaderMap headers = resourceRequest.httpHeaderFields(); >+ if (!headers.isEmpty()) { >+ HTTPHeaderMap::const_iterator end = headers.end(); >+ for (HTTPHeaderMap::const_iterator it = headers.begin(); it != end; ++it) >+ g_hash_table_insert(priv->http_headers, (gpointer)g_strdup(it->first.utf8().data()), (gpointer)g_strdup(it->second.utf8().data())); >+ } Maybe it's better to lazily initialise the hash table only when needed. >+/** >+ * webkit_network_request_new: >+ * @uri: an already encoded URI >+ * >+ * Creates a new #WebKitNetworkRequest initialized with an URI. >+ * >+ * Returns: a new #WebKitNetworkRequest >+ */ > WebKitNetworkRequest* webkit_network_request_new(const gchar* uri) Why is this function public? Does it make sense to use it in an application? I cannot think to any use case so maybe we could think about deprecating it. What do you mean with "already encoded URI"? >+const GHashTable* webkit_network_request_get_http_headers(WebKitNetworkRequest* request) >+{ >+ g_return_val_if_fail(WEBKIT_IS_NETWORK_REQUEST(request), NULL); >+ >+ WebKitNetworkRequestPrivate* priv = request->priv; >+ return priv->http_headers; >+} The documentation is missing. Maybe it could be useful to be able to keep the hash table somewhere after calling _ref, but in this case the return value should not be const and maybe people would think that they can overwrite values directly on the hash table. What do you think? >+/** >+ * WebKitReqCachePolicy: >+ * @WEBKIT_REQ_CACHE_POLICY_NONE: invalid, marker policy >[...] >+ */ >+typedef enum >+{ >+ WEBKIT_REQ_CACHE_POLICY_NONE, I don't think we need it. Do we need all the getters and setters for headers? Like webkit_network_request_[gs]et_http_user_agent(). Probably the various things for which we have getters and setters (excluding the hash table for headers and probably also the body as it needs also the length to be usable) should also be properties. (In reply to comment #10) > >struct _WebKitNetworkRequestPrivate { > >+ ResourceRequest request; > >+ > > gchar* uri; > >+ WebKitReqCachePolicy cache_policy; > >+ gdouble timeout_interval; > >+ gchar* main_document_uri; > >+ gchar* http_method; > >+ GHashTable *http_headers; > >+ gboolean allow_http_cookies; > >+ gchar *body; > >+ gsize body_size; > > }; > > You should always use the WebKit style for things that are not exposed in the > public API. > > Why are you keeping in WebKitNetworkRequestPrivate both the ResourceRequest > and the values (like cache_policy) that can be easily extracted from it? > Maybe it makes sense for strings to avoid conversions and duplications > but not for just integers. I second that, It makes sense to use members directly, maybe even for strings. > >+/** > >+ * webkit_network_request_new: > >+ * @uri: an already encoded URI > >+ * > >+ * Creates a new #WebKitNetworkRequest initialized with an URI. > >+ * > >+ * Returns: a new #WebKitNetworkRequest > >+ */ > > WebKitNetworkRequest* webkit_network_request_new(const gchar* uri) > > Why is this function public? Does it make sense to use it in an application? I > cannot think to any use case so maybe we could think about deprecating it. Think of "webkit_web_frame_load_request" here. In that combinaison it seems like a very powerful interface to me. > What do you mean with "already encoded URI"? Scratch "already encoded". If it's an URI, it must be encoded, everything else is ambiguous. :) (In reply to comment #10) > Maybe it could be useful to be able to keep the hash table somewhere after > calling _ref, but in this case the return value should not be const and maybe > people would think that they can overwrite values directly on the hash table. > What do you think? I think use cases that would require keeping the headers are few enough and should be rare enough that it is better to keep the const nature of the hash table instead of making this API a bit confusing. It's a small numbers of headers to copy if needed, too. Created attachment 22531 [details]
proposed patch
Another go. I removed the two signals as requested, I agree it's best to discuss this with the big picture in mind. As for the name of a single, unified signal. What about something like navigation-started, since it is caused by a navigation-requested, AFAIK?
(In reply to comment #13) > + ResourceRequest requestCopy = resourceRequest; > + WebKitNetworkRequest* request = webkit_network_request_new_from_resource_request(requestCopy); Why a copy is then the request is copied again to priv->request? > +gdouble webkit_network_request_get_timeout_interval(WebKitNetworkRequest* request) > +{ > + g_return_val_if_fail(WEBKIT_IS_NETWORK_REQUEST(request), 0.0); > + > + WebKitNetworkRequestPrivate* priv = request->priv; > + > + return (gdouble)priv->request.timeoutInterval(); > +} You don't need the cast from double to gdouble. > +const GHashTable* webkit_network_request_get_http_headers(WebKitNetworkRequest* request) > +{ > + g_return_val_if_fail(WEBKIT_IS_NETWORK_REQUEST(request), NULL); > + > + return webkit_network_request_private_get_http_headers(request); > +} Why do you have a separate webkit_network_request_private_get_http_headers? > + * For instance: if the request already has a X-Misc header with a > + * value of "go, webkit!", and we add the same X-Misc header > + * containing "go!" we will end up having a single X-Misc header with > + * a value of "go, webkit!,go!". Maybe the example would be clearer if you use a real header, like Accept-Language. > +void webkit_network_response_set_uri(WebKitNetworkResponse* response, const gchar* uri) > +{ > + g_return_if_fail(WEBKIT_IS_NETWORK_RESPONSE(response)); > + g_return_if_fail(uri); > + > + WebKitNetworkResponsePrivate* priv = response->priv; > + > + g_free(priv->uri); > + priv->uri = g_strdup(uri); > + > + /* > + * feeding back to the ResourceResponse object, which is the one actually used > + * inside WebCore > + */ > + priv->response.setUrl(KURL(String::fromUTF8(uri))); > +} g_object_notify is missing. Can a native English speaker check if the documentation is ok? (In reply to comment #14) > Why a copy is then the request is copied again to priv->request? My bad. > Why do you have a separate webkit_network_request_private_get_http_headers? I can't use get_http_headers directly because of the const qualifier. And I still need a function so that I can lazily fill the hash table. > > + priv->response.setUrl(KURL(String::fromUTF8(uri))); > > +} > > g_object_notify is missing. NetworkResponse doesn't have properties, et al yet, and I plan to finish its implementation after NetworkRequest is done. I created a skeleton just because I needed a class to provide the signal with. (In reply to comment #15) > > Why do you have a separate webkit_network_request_private_get_http_headers? > > I can't use get_http_headers directly because of the const qualifier. And I > still need a function so that I can lazily fill the hash table. Oh, right. > > > + priv->response.setUrl(KURL(String::fromUTF8(uri))); > > > +} > > > > g_object_notify is missing. > > NetworkResponse doesn't have properties, et al yet, and I plan to finish its > implementation after NetworkRequest is done. I created a skeleton just because > I needed a class to provide the signal with. Ehm... I didn't notice that ;) Created attachment 22545 [details]
proposed patch
I think I addressed all of the comments now.
Created attachment 22557 [details]
proposed patch
Christian Dywan advised me in another bug to add 'Since' tags to the documenation of the new created public methods. I decided to add them to this patch too, and took the oportunity to do some housekeeping and make sure everything is tidied up.
Comment on attachment 22557 [details]
proposed patch
Assigning to Alp for review or for re-assignment to the appropriate Gtk reviewer. Most of us don't know anything about Gtk, so it's silly for this to be in the general review queue.
(In reply to comment #18) > Created an attachment (id=22557) [review] > proposed patch Since we plan to actually expose libSoup API now, would it make sense to expose a SoupMessage here? It looks like WebKitNetworkRequest to some extend reflects what a 'message' does. While I doubt that either one can replace the other, a 'request' might for instance contain a 'message', for things like the method, uri, body and headers. But that's only an idea, it might be less good an idea in practise. Another point is caching, we only have memory caching. It might be good to figure out what the plan is, ie. if libSoup is going to feature caching, or if we rely on a proxy server to do that, or if WebKit itself should take care of it. And maybe refrain from introducing an option unless we know what it is going to affect. (In reply to comment #20) > (In reply to comment #18) > > Created an attachment (id=22557) [review] [review] > > proposed patch > > Since we plan to actually expose libSoup API now, would it make sense to expose > a SoupMessage here? It looks like WebKitNetworkRequest to some extend reflects > what a 'message' does. While I doubt that either one can replace the other, a > 'request' might for instance contain a 'message', for things like the method, > uri, body and headers. But that's only an idea, it might be less good an idea > in practise. > > Another point is caching, we only have memory caching. It might be good to > figure out what the plan is, ie. if libSoup is going to feature caching, or if > we rely on a proxy server to do that, or if WebKit itself should take care of > it. And maybe refrain from introducing an option unless we know what it is > going to affect. > I like the idea. Though I believe that we actually need to have the Message become a member of ResourceRequest (or ResourceHandle?) itself, and not be created in ResourceHandle::startHttp. We also need to figure out how to deal with non-HTTP stuff; should we have some kind of handling for that in ResourceRequest and WebKitNetworkRequest? This could be implemented in a way that the SoupMessage will only be instantiated when a .soupMessage() accessor method is called in ResourceRequest, and the good thing is this allows our users to access the SoupMessage (or GFile, in case of a local request, which will probably force us to decide on this earlier?) which will be used by the Soup backend, at willSendRequest already. Created attachment 26755 [details] Minimal functionality addition patch I rewrote my patch so that only the minimal functionality needed to get HTTP information correctly fed back into WebKit when client code passes along a WebKitNetworkRequest it received in a signal. This allows us to make it possible for browsers such as Epihany and Midori to open links in new tabs and still get Referer, for instance, correctly sent. Internal functionality (in WebkitWebFrame) is also improved. See comments bellow http://bugzilla.gnome.org/show_bug.cgi?id=120341#c12 for more reasoning. I provide this patch now so that we can get this working sooner rather than later, and no public API is touched, so we can discuss the merits of my proposed API later. Created attachment 26759 [details]
Minimal functionality addition patch
Small change requested by xan to avoid having 2 returns in 4 lines ;)
Created attachment 26912 [details]
proof of concept, so that we can discuss exposing SoupMessage
This isn't ready for reviewing yet, so I'm not bothering adding a ChangeLog entry, and am adding GtkLauncher code to test. We need to figure out how to handle modifications of SoupMessage to apply to the actual request, and document them. It seems like modifying headers of the SoupMessage in the navigation-policy-decision-requested callback doesn't affect the actual request, for instance (even if I force an update of ResourceRequest from the SoupMessage).
(In reply to comment #24) > Created an attachment (id=26912) [review] > proof of concept, so that we can discuss exposing SoupMessage > > This isn't ready for reviewing yet, so I'm not bothering adding a ChangeLog > entry, and am adding GtkLauncher code to test. We need to figure out how to > handle modifications of SoupMessage to apply to the actual request, and > document them. It seems like modifying headers of the SoupMessage in the > navigation-policy-decision-requested callback doesn't affect the actual > request, for instance (even if I force an update of ResourceRequest from the > SoupMessage). Hm.. I would think it's intuitive to modify the message and expect changes to take effect as if one had sent the message outside of WebKit. So if we can arrange that somehow that'd be great, not sure if there is any reason against that from WebCore. And just so it's not forgotten, an according property should be nice. Created attachment 26935 [details]
minimal functionality
This is a reworked patch to carry a pointer to the actual ResourceRequest instead of a copy.
Created attachment 26937 [details]
proof of concept for exposing SoupMessage
I dislike the fact that the user will have to call webkit_network_request_refresh(), but it seems to be necessary right now, because soup doesn't provide means to know when headers got changed in any way.
Created attachment 28342 [details]
minimal functionality
Updated to current code, and fixed style issues I was able to detect myself ;).
(In reply to comment #28) > Created an attachment (id=28342) [review] > minimal functionality > > Updated to current code, and fixed style issues I was able to detect myself ;). > I forgot to update the ChangeLog after updating the patch, so some functions/classes that were edited are missing the auto-generated list. I have it up-to-date on my git branch, though, and will commit the correct one, if the patch is r+'ed. (In reply to comment #29) > (In reply to comment #28) > > Created an attachment (id=28342) [review] [review] > > minimal functionality > > > > Updated to current code, and fixed style issues I was able to detect myself ;). > > > > I forgot to update the ChangeLog after updating the patch, so some > functions/classes that were edited are missing the auto-generated list. I have > it up-to-date on my git branch, though, and will commit the correct one, if > the patch is r+'ed. I like how the patch works, keeping the request instead of only the URI is clearly much better. Created attachment 28877 [details]
minimal funcionality
Tired of seeing patches using just the uri from the request, I'll try to push this and be done with it =).
Created attachment 28878 [details]
expose SoupMessage
+#include <libsoup/soup.h> +#include <libsoup/soup-message.h> Does libsoup actually require this? In that case *please* file a bug against libsoup to fix the include files. =) + * Gets the #SoupMessage held and used by the given request. Notice + * that most times WebKitGTK+ gives you a #WebKitNetworkRequest in + * signals, it's for informational purposes only. If you intend to + * modify the headers or data sent in a request manually, please read + * the documentation of the signal you are handling to see if that is + * supposed to work. I think this is a bit vague. Maybe we want to reword it slightly to stress that the message can always be read but only if documented as such do we support modifications. I think if we decide to speak about supported use cases it gives the API user an idea of relibability, whereas "supposed to work" gives the impression that it may work and break in any revision: + * Gets the #SoupMessage held and used by the given request. Notice + * that the message is mainly available for informational purposes. + * If you intend to modify the headers or data sent in a request + * manually, please read the documentation of the signal you are + * handling to see if that is supported. Always allowing to at least read out any information is already powerful, so if that speeds up review, we can discuss documentation of the supported cases where modifications are allowed as a follow up. + ResourceRequest* resourceRequest = new ResourceRequest(KURL(String::fromUTF8(uri))); Please use the two-arg KURL ctor, see https://bugs.webkit.org/show_bug.cgi?id=23761 for rationale. + SoupURI *soupURI = soup_message_get_uri(m_soupMessage);
+
+ gchar *uri = soup_uri_to_string(soupURI, FALSE);
The * are all wrong :)
+ m_url = KURL(String::fromUTF8(uri));
two-arg ctor.
+ while (soup_message_headers_iter_next(&headersIter, (const gchar**)&headerName, (const gchar**)&headerValue)) {
C++ style castings?
+ gchar *headerName = 0;
+ gchar *headerValue = 0;
No need to initialize? And * is wrong.
+ SoupURI *soupURI = soup_uri_new(url().string().utf8().data());
+ soup_message_set_uri(m_soupMessage, soupURI);
Leaking soupURI (and * is wrong).
Stupid question: is it OK to free the message in doUpdate the way you do it? Shouldn't it go in a destructor or something similar?
(And as said only including <libsoup/soup.h> works)
And I guess now it's too late to ask about that conversation we had about using directly the soup objects/API without any wrappers like the other ports do?
(In reply to comment #33) > + * Gets the #SoupMessage held and used by the given request. Notice > + * that most times WebKitGTK+ gives you a #WebKitNetworkRequest in > + * signals, it's for informational purposes only. If you intend to > + * modify the headers or data sent in a request manually, please read > + * the documentation of the signal you are handling to see if that is > + * supposed to work. > > I think this is a bit vague. Maybe we want to reword it slightly to stress that > the message can always be read but only if documented as such do we support > modifications. I think if we decide to speak about supported use cases it gives > the API user an idea of relibability, whereas "supposed to work" gives the > impression that it may work and break in any revision: Right. I will reword this a bit. Just for completeness, like I said in our IRC conversation, modification of the soup message will probably only be supported by the yet-to-be-implemented signal that is emitted when FrameLoaderClient's willSendRequest delegate is called. (In reply to comment #35) > Stupid question: is it OK to free the message in doUpdate the way you do it? > Shouldn't it go in a destructor or something similar? It looks OK to me. The thing is the request may have been "nullified", but not destroyed, and that code block handles this case. > (And as said only including <libsoup/soup.h> works) Right, thanks! > And I guess now it's too late to ask about that conversation we had about using > directly the soup objects/API without any wrappers like the other ports do? We are already using WebKitNetworkRequest objects all over the code. It may be a good thing to have it, anyway, if we need to extend the functionality of the object to do something that would not fit in libsoup. Just building to check if the fixed up patches are OK to upload, thanks for the comments! Created attachment 28889 [details]
Make our NetworkRequest object functionally complete, by making it
carry the ResourceRequest it represents with it.
---
WebKit/gtk/ChangeLog | 28 +++++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 7 +-
WebKit/gtk/webkit/webkitdownload.cpp | 4 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 78 ++++++++++++++++++-
WebKit/gtk/webkit/webkitprivate.h | 11 +++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
6 files changed, 117 insertions(+), 15 deletions(-)
Created attachment 28890 [details]
Expose the SoupMessage of the request through our WebKitNetworkRequest
object.
---
WebCore/ChangeLog | 21 +++++
WebCore/GNUmakefile.am | 1 +
.../platform/network/soup/ResourceHandleSoup.cpp | 10 +--
WebCore/platform/network/soup/ResourceRequest.h | 19 ++++-
.../platform/network/soup/ResourceRequestSoup.cpp | 91 ++++++++++++++++++++
WebKit/gtk/ChangeLog | 14 +++
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 21 +++++
WebKit/gtk/webkit/webkitnetworkrequest.h | 4 +
8 files changed, 170 insertions(+), 11 deletions(-)
(In reply to comment #36) > (In reply to comment #33) > > + * Gets the #SoupMessage held and used by the given request. Notice > > + * that most times WebKitGTK+ gives you a #WebKitNetworkRequest in > > + * signals, it's for informational purposes only. If you intend to > > + * modify the headers or data sent in a request manually, please read > > + * the documentation of the signal you are handling to see if that is > > + * supposed to work. > > > > I think this is a bit vague. Maybe we want to reword it slightly to stress that > > the message can always be read but only if documented as such do we support > > modifications. I think if we decide to speak about supported use cases it gives > > the API user an idea of relibability, whereas "supposed to work" gives the > > impression that it may work and break in any revision: > > Right. I will reword this a bit. Just for completeness, like I said in our IRC > conversation, modification of the soup message will probably only be supported > by the yet-to-be-implemented signal that is emitted when FrameLoaderClient's > willSendRequest delegate is called. Mac actually supports this in their API. For example the WebDataSource::resource method has the following doc: "The URL returned may be different from the original request. A WebView’s resource load delegate may modify requests by implementing webView:resource:willSendRequest:redirectResponse:fromDataSource:" Comment on attachment 28889 [details] Make our NetworkRequest object functionally complete, by making it > diff --git a/WebKit/gtk/webkit/webkitdownload.cpp b/WebKit/gtk/webkit/webkitdownload.cpp > index 4488304..3bceedc 100644 > --- a/WebKit/gtk/webkit/webkitdownload.cpp > +++ b/WebKit/gtk/webkit/webkitdownload.cpp > @@ -416,9 +416,7 @@ void webkit_download_start(WebKitDownload* download) > if (priv->resourceHandle) > priv->resourceHandle->setClient(priv->downloadClient); > else { > - // FIXME: Use the actual request object when WebKitNetworkRequest is finished. > - ResourceRequest request(webkit_network_request_get_uri(priv->networkRequest)); > - priv->resourceHandle = ResourceHandle::create(request, priv->downloadClient, 0, false, false, false); > + priv->resourceHandle = ResourceHandle::create(*webkit_network_request_get_core_request(priv->networkRequest), priv->downloadClient, 0, false, false, false); Should this be core(priv->networkRequest)? For objects that wrap WebCore objects, we use core/kit to get the respective objects. > > +// The mustFreeRequest variable here is used to store if the > +// ResourceRequest we are storing is owned by us or by WebCore; this > +// won't be needed when/if ResourceRequest is reference counted. > struct _WebKitNetworkRequestPrivate { > gchar* uri; Can we use CString here? WebKitWebHistoryItem uses CStrings to make memory management easier. > +ResourceRequest* webkit_network_request_get_core_request(const WebKitNetworkRequest* request) > +{ Change this to core? > + WebKitNetworkRequest* > + webkit_network_request_new_with_core_request(const WebCore::ResourceRequest& resourceRequest); > + > + WebCore::ResourceRequest* > + webkit_network_request_get_core_request(const WebKitNetworkRequest* request); > + Make this 'core'. > - // TODO: Use the ResourceRequest carried by WebKitNetworkRequest when it is implemented. > - String string = String::fromUTF8(webkit_network_request_get_uri(request)); > - coreFrame->loader()->load(ResourceRequest(KURL(KURL(), string)), false); > + coreFrame->loader()->load(const_cast<ResourceRequest&>(*webkit_network_request_get_core_request(request)), false); same - core. (In reply to comment #40) > Mac actually supports this in their API. For example the > WebDataSource::resource method has the following doc: > > "The URL returned may be different from the original request. A WebView’s > resource load delegate may modify requests by implementing > webView:resource:willSendRequest:redirectResponse:fromDataSource:" > Yep. My original patch also implemented willSendRequest as a signal, but we decided to remove it from this patch, and add it later, or through the frameloaders rework. (In reply to comment #41) > Should this be core(priv->networkRequest)? For objects that wrap WebCore > objects, we use core/kit to get the respective objects. I had written that change, but went back on it because there would be no obvious way of implementing a matching kit(). I have done it for the patch I'm about to post, though. We can think of a good way of implemementing kit() afterwards. > Can we use CString here? WebKitWebHistoryItem uses CStrings to make memory > management easier. After writing half the patch, I decided using CString wouldn't make the code cleaner at all, in my opinion, so I would prefer not changing the way it looks like right now, regarding this, is that OK with you? Created attachment 28913 [details]
Make our NetworkRequest object functionally complete, by making it
carry the ResourceRequest it represents with it.
---
WebKit/gtk/ChangeLog | 28 +++++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 7 +-
WebKit/gtk/webkit/webkitdownload.cpp | 4 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 78 ++++++++++++++++++-
WebKit/gtk/webkit/webkitprivate.h | 11 +++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
6 files changed, 117 insertions(+), 15 deletions(-)
Comment on attachment 28913 [details]
Make our NetworkRequest object functionally complete, by making it
Most comments by janm adresed.
There has been no activity on this bug in over 2 months. Can we assume it's dead and close it? Please advise. janm said over IRC he would review these tonight when he gets home. Comment on attachment 28890 [details] Expose the SoupMessage of the request through our WebKitNetworkRequest > diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am > index ffaf1b2..34f2d9a 100644 > --- a/WebCore/GNUmakefile.am > +++ b/WebCore/GNUmakefile.am > @@ -1839,6 +1839,7 @@ webcore_sources += \ > WebCore/platform/network/soup/DNSSoup.cpp \ > WebCore/platform/network/soup/ResourceError.h \ > WebCore/platform/network/soup/ResourceHandleSoup.cpp \ > + WebCore/platform/network/soup/ResourceRequestSoup.cpp \ Don't forget to add this in webcoregtk_sources instead. > diff --git a/WebCore/platform/network/soup/ResourceRequestSoup.cpp b/WebCore/platform/network/soup/ResourceRequestSoup.cpp > new file mode 100644 > index 0000000..088b7bd > --- /dev/null > +++ b/WebCore/platform/network/soup/ResourceRequestSoup.cpp > + > +void ResourceRequest::doUpdateResourceRequest() > +{ > + SoupURI* soupURI = soup_message_get_uri(m_soupMessage); > + > + gchar* uri = soup_uri_to_string(soupURI, FALSE); > + m_url = KURL(KURL(), String::fromUTF8(uri)); > + g_free(uri); > + > + m_httpMethod = String::fromUTF8(m_soupMessage->method); > + > + m_httpHeaderFields.clear(); We'll be losing what's already in the header fields if we clear this. Is there a reason why this should be cleared? > + > + SoupMessageHeadersIter headersIter; > + const char* headerName; > + const char* headerValue; > + > + soup_message_headers_iter_init(&headersIter, m_soupMessage->request_headers); > + while (soup_message_headers_iter_next(&headersIter, &headerName, &headerValue)) > + m_httpHeaderFields.set(String::fromUTF8(headerName), String::fromUTF8(headerValue)); > + > + m_httpBody = FormData::create(m_soupMessage->request_body->data, m_soupMessage->request_body->length); > +} what about m_allowHTTPCookies and m_alloFirstPartyForCookies? Do we need to update them too? > + > +void ResourceRequest::doUpdatePlatformRequest() > +{ > + if (isNull()) { > + if (m_soupMessage) > + g_object_unref(m_soupMessage); > + m_soupMessage = 0; > + return; > + } > + > + if (!m_soupMessage) > + m_soupMessage = soup_message_new(httpMethod().utf8().data(), url().string().utf8().data()); > + else { > + SoupURI* soupURI = soup_uri_new(url().string().utf8().data()); > + soup_message_set_uri(m_soupMessage, soupURI); > + soup_uri_free(soupURI); > + > + g_object_set(m_soupMessage, "method", httpMethod().utf8().data(), NULL); > + } > + soup_message_headers_clear(m_soupMessage->request_headers); > + > + HTTPHeaderMap headers = httpHeaderFields(); > + SoupMessageHeaders *soupHeaders = m_soupMessage->request_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()); > + } Same question re updating cookies. > + > + // Body data is only handlded at ResourceHandleSoup::startHttp Is there a reason why we're doing it there? > diff --git a/WebKit/gtk/webkit/webkitnetworkrequest.cpp b/WebKit/gtk/webkit/webkitnetworkrequest.cpp > index 1e3ef2c..dc50fef 100644 > --- a/WebKit/gtk/webkit/webkitnetworkrequest.cpp > +++ b/WebKit/gtk/webkit/webkitnetworkrequest.cpp > @@ -174,4 +174,25 @@ G_CONST_RETURN gchar* webkit_network_request_get_uri(WebKitNetworkRequest* reque > return priv->uri; > } > > +/** > + * webkit_network_request_get_soup_message: Can't we just use get_message here to make the name not depend on the library we're using? The return type already specify that it's a SoupMessage so it's just making it redundant by adding _soup_ to the function name. > + * @request: a #WebKitNetworkRequest > + * > + * Obtains the #SoupMessage held and used by the given request. Notice > + * that modification of the SoupMessage of a request by signal > + * handlers is only supported where explicitly documented. Can you expound or rephrase this? I don't quite understand what the doc mean here in terms of modifying the SoupMessage. Patch is fine nevertheless. Another gtk+ reviewer needs to ACK on the API too. (In reply to comment #48) > > + WebCore/platform/network/soup/ResourceRequestSoup.cpp \ > > Don't forget to add this in webcoregtk_sources instead. OK! > > +void ResourceRequest::doUpdateResourceRequest() [...] > > + m_httpHeaderFields.clear(); > > We'll be losing what's already in the header fields if we clear this. Is there > a reason why this should be cleared? This function assumes that the ResourceRequest information is outdated and needs to be updated from the SoupMessage. There should be no case in which both are 'dirty', so clearing what is in the ResourceRequest object is the right way to go here. Having said that, no other port does it, it seems, so it may not be needed, and I'm OK with removing this call. > > + m_httpBody = FormData::create(m_soupMessage->request_body->data, m_soupMessage->request_body->length); > > +} > > what about m_allowHTTPCookies and m_alloFirstPartyForCookies? Do we need to > update them too? We don't do anything with those right now, and I'm not sure how this could be implemented. I'd prefer handling this in a later patch, if need be. > > + > > + // Body data is only handlded at ResourceHandleSoup::startHttp > > Is there a reason why we're doing it there? That's mostly me being careful for this first patch. Body data handling is kinda complex in our port, possibly involving mmaping files and stuff, I preferred leaving it where it is. I remember having a specific problem, but it's gone from my mind right now. > Can't we just use get_message here to make the name not depend on the library > we're using? The return type already specify that it's a SoupMessage so it's > just making it redundant by adding _soup_ to the function name. Sounds good to me. I have no strong feelings regarding this. > > + * Obtains the #SoupMessage held and used by the given request. Notice > > + * that modification of the SoupMessage of a request by signal > > + * handlers is only supported where explicitly documented. > > Can you expound or rephrase this? I don't quite understand what the doc mean > here in terms of modifying the SoupMessage. This is because changes to the ResourceRequest are only supported in willSendRequest. You can fetch the SoupMessage while handling stuff such as navigation-policy-decision-requested, but it's not guaranteed that your changes will do anything. So we would document, in the signal mapping willSendRequest, that you can edit the message there. Comment on attachment 28913 [details]
Make our NetworkRequest object functionally complete, by making it
As kov suggested, it would be better to grab a SoupMessage ref from ResourceRequest rather than carrying a ResourceRequest around.
r- as discussed to have this patch reworked.
Comment on attachment 28890 [details] Expose the SoupMessage of the request through our WebKitNetworkRequest Patch needs rework due to https://bugs.webkit.org/attachment.cgi?id=28913 being reworked. r- for now. Created attachment 30604 [details]
Make NetworkRequest carry a reference of the SoupMessage used by ResourceRequest
WebCore/ChangeLog | 18 ++++
WebCore/GNUmakefile.am | 1 +
.../platform/network/soup/ResourceHandleSoup.cpp | 10 +--
WebCore/platform/network/soup/ResourceRequest.h | 19 ++++-
.../platform/network/soup/ResourceRequestSoup.cpp | 93 ++++++++++++++++++++
WebKit/gtk/ChangeLog | 23 +++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 6 +-
WebKit/gtk/webkit/webkitdownload.cpp | 7 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 44 +++++++++-
WebKit/gtk/webkit/webkitprivate.h | 12 +++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
11 files changed, 213 insertions(+), 24 deletions(-)
Comment on attachment 30604 [details] Make NetworkRequest carry a reference of the SoupMessage used by ResourceRequest > + ResourceRequest(SoupMessage* soupMessage) > + : ResourceRequestBase() > + , m_soupMessage(soupMessage) > + { > + } > + Would be nice if we can initialize ResourceRequestBast as ResourceRequestBase(KURL(), UseProtocolCachePolicy) > +void ResourceRequest::doUpdateResourceRequest() > +{ > + SoupURI* soupURI = soup_message_get_uri(m_soupMessage); We need to null-check m_soupMessage here just in case updatePlatformRequest hasn't been called. > + gchar* uri = soup_uri_to_string(soupURI, FALSE); > + m_url = KURL(KURL(), String::fromUTF8(uri)); > + g_free(uri); no need to create uri. Just put the call inside fromUTF8(). > + > + m_httpBody = FormData::create(m_soupMessage->request_body->data, m_soupMessage->request_body->length); > +} Can we add a FIXME for the cookies stuff to be updated once we figure out how to do that? Thanks. > + > +void ResourceRequest::doUpdatePlatformRequest() > +{ > + if (isNull()) { > + if (m_soupMessage) > + g_object_unref(m_soupMessage); > + m_soupMessage = 0; > + return; > + } > + > + if (!m_soupMessage) { > + m_soupMessage = soup_message_new(httpMethod().utf8().data(), url().string().utf8().data()); > + > + if (!m_soupMessage) > + return; > + } else { > + SoupURI* soupURI = soup_uri_new(url().string().utf8().data()); > + soup_message_set_uri(m_soupMessage, soupURI); > + soup_uri_free(soupURI); I think we can avoid initializing a soupURI here by moving soup_uri_new in the call to soup_message_set_uri(). > + > + g_object_set(m_soupMessage, "method", httpMethod().utf8().data(), NULL); > + > + soup_message_headers_clear(m_soupMessage->request_headers); > + } I think we can lose the else block since here since we only need to make sure we have a m_soupMessage before doing anything. > + > + HTTPHeaderMap headers = httpHeaderFields(); > + SoupMessageHeaders* soupHeaders = m_soupMessage->request_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 only handlded at ResourceHandleSoup::startHttp handlded -> handled. Maybe a good idea to put the rationale here why we're doing it in startHttp(). Looking good. I'm going to r- this for now until the trivial issues above are addressed. Cheers (In reply to comment #53) > (From update of attachment 30604 [details] [review]) > > + ResourceRequest(SoupMessage* soupMessage) > > + : ResourceRequestBase() > > + , m_soupMessage(soupMessage) > > + { > > + } > > + > > Would be nice if we can initialize ResourceRequestBast as > ResourceRequestBase(KURL(), UseProtocolCachePolicy) Why? The parameter-less variant is there for this specific use case (creating a ResourceRequest from the platform network object), so I think it is the appropriate one. > > +void ResourceRequest::doUpdateResourceRequest() > > +{ > > + SoupURI* soupURI = soup_message_get_uri(m_soupMessage); > > We need to null-check m_soupMessage here just in case updatePlatformRequest > hasn't been called. Or has returned without being able to create the message for some reason. > > + gchar* uri = soup_uri_to_string(soupURI, FALSE); > > + m_url = KURL(KURL(), String::fromUTF8(uri)); > > + g_free(uri); > > no need to create uri. Just put the call inside fromUTF8(). I think we need. fromUTF8 will copy the string, so we need to free the original somehow. Perhaps we can use GOwnPtr. > Can we add a FIXME for the cookies stuff to be updated once we figure out how > to do that? Thanks. Sure. > > + SoupURI* soupURI = soup_uri_new(url().string().utf8().data()); > > + soup_message_set_uri(m_soupMessage, soupURI); > > + soup_uri_free(soupURI); > > I think we can avoid initializing a soupURI here by moving soup_uri_new in the > call to soup_message_set_uri(). Same problem as with the uri string. > I think we can lose the else block since here since we only need to make sure > we have a m_soupMessage before doing anything. Right! > handlded -> handled. Maybe a good idea to put the rationale here why we're > doing it in startHttp(). ok Created attachment 30623 [details]
Make NetworkRequest carry a reference of the SoupMessage used by
ResourceRequest
---
WebCore/ChangeLog | 22 ++++
WebCore/GNUmakefile.am | 1 +
.../platform/network/soup/ResourceHandleSoup.cpp | 42 ++++----
WebCore/platform/network/soup/ResourceRequest.h | 29 +++++-
.../platform/network/soup/ResourceRequestSoup.cpp | 114 ++++++++++++++++++++
WebKit/gtk/ChangeLog | 24 ++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 6 +-
WebKit/gtk/webkit/webkitdownload.cpp | 7 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 51 +++++++++-
WebKit/gtk/webkit/webkitprivate.h | 12 ++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
11 files changed, 274 insertions(+), 38 deletions(-)
Comment on attachment 30623 [details]
Make NetworkRequest carry a reference of the SoupMessage used by
One more try. Notice that most of the improved life cycle checks and safe guards are needed because the SoupMessage is not limited to being used inside ResourceHandle. We had an implicit assumption, for instance, that the message would be gone after finishedCallback, and that may not be true now.
Created attachment 30634 [details]
Make NetworkRequest carry a reference of the SoupMessage used by
ResourceRequest
---
ChangeLog | 9 ++
GNUmakefile.am | 8 +-
WebCore/ChangeLog | 23 +++
WebCore/GNUmakefile.am | 1 +
.../platform/network/soup/ResourceHandleSoup.cpp | 48 ++++---
WebCore/platform/network/soup/ResourceRequest.h | 32 ++++-
.../platform/network/soup/ResourceRequestSoup.cpp | 142 ++++++++++++++++++++
WebKit/gtk/ChangeLog | 24 ++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 6 +-
WebKit/gtk/webkit/webkitdownload.cpp | 7 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 51 +++++++-
WebKit/gtk/webkit/webkitprivate.h | 13 ++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
13 files changed, 329 insertions(+), 39 deletions(-)
Comment on attachment 30634 [details]
Make NetworkRequest carry a reference of the SoupMessage used by
This time with a simple unit test. It needs to be enhanced to use something such as a SoupServer. Using something that is not an HTTP server is no good, since that is exactly what we want to try out.
Comment on attachment 30634 [details] Make NetworkRequest carry a reference of the SoupMessage used by > diff --git a/WebCore/platform/network/soup/ResourceRequestSoup.cpp b/WebCore/platform/network/soup/ResourceRequestSoup.cpp > +template <> void freeOwnedGPtr<SoupURI>(SoupURI* uri) > +{ > + if (uri) > + soup_uri_free(uri); Please fix the spacing here. > +using namespace std; Please move this after the include so it's more visible. > +ResourceRequest::ResourceRequest(const ResourceRequest& resourceRequest) > + : ResourceRequestBase(resourceRequest) > + , m_soupMessage(0) > +{ > + m_url = resourceRequest.url().copy(); > + m_firstPartyForCookies = resourceRequest.firstPartyForCookies().copy(); > + m_httpMethod = resourceRequest.httpMethod().copy(); > + m_httpHeaderFields.adopt(auto_ptr<CrossThreadHTTPHeaderMapData>(resourceRequest.m_httpHeaderFields.copyData().release())); Please convert this to use PassOwnPtr instead of auto_ptr. > + > + size_t encodingCount = resourceRequest.m_responseContentDispositionEncodingFallbackArray.size(); > + if (encodingCount > 0) { > + String encoding1 = resourceRequest.m_responseContentDispositionEncodingFallbackArray[0]; > + String encoding2; > + String encoding3; > + if (encodingCount > 1) { > + encoding2 = resourceRequest.m_responseContentDispositionEncodingFallbackArray[1]; > + if (encodingCount > 2) > + encoding3 = resourceRequest.m_responseContentDispositionEncodingFallbackArray[2]; > + } > + ASSERT(encodingCount <= 3); > + setResponseContentDispositionEncodingFallbackArray(encoding1, encoding2, encoding3); > + } > + if (resourceRequest.m_httpBody) > + setHTTPBody(resourceRequest.httpBody()); > + > + m_resourceRequestUpdated = true; > + m_platformRequestUpdated = false; should these be in updatePlatformRequest instead of the constructor? > +void ResourceRequest::doUpdatePlatformRequest() > +{ > + if (isNull()) { > + if (m_soupMessage) > + g_object_unref(m_soupMessage); > + m_soupMessage = 0; > + return; > + } Are we being too aggressive here? Is the call in ResourceRequest's destructor not enough? > + > + GOwnPtr<SoupURI> soupURI(soup_uri_new(url().string().utf8().data())); > + ASSERT(soupURI.get()); ASSERT(soupURI) should be enough. Looks good. > > + m_resourceRequestUpdated = true; > > + m_platformRequestUpdated = false; > > should these be in updatePlatformRequest instead of the constructor? No, these should be set in the constructor (and setters, wherever it makes sense). updatePlatformRequest, and updateResourceRequest checks for them to figure out if it should do anything at all, so setting them there is wrong. > > +void ResourceRequest::doUpdatePlatformRequest() > > +{ > > + if (isNull()) { > > + if (m_soupMessage) > > + g_object_unref(m_soupMessage); > > + m_soupMessage = 0; > > + return; > > + } > > Are we being too aggressive here? Is the call in ResourceRequest's destructor > not enough? I believe so. I checked other ports code, and they have no code dealing with this code. I can't see this case happening, from my reading ot the code, too. I'll try the code with removing this block of code for a bit to see if I spot any crashes. > > + GOwnPtr<SoupURI> soupURI(soup_uri_new(url().string().utf8().data())); > > + ASSERT(soupURI.get()); > > ASSERT(soupURI) should be enough. > > Looks good. So r=you, with PassOwnPtr, the style fixes, my clarification on the member variables initialization, the isNull() code removed, and this change? ;) Let me upload the updated patch. Created attachment 30763 [details]
network request
ChangeLog | 9 ++
GNUmakefile.am | 8 +-
WebCore/ChangeLog | 23 ++++
WebCore/GNUmakefile.am | 1 +
.../platform/network/soup/ResourceHandleSoup.cpp | 48 ++++---
WebCore/platform/network/soup/ResourceRequest.h | 32 +++++-
.../platform/network/soup/ResourceRequestSoup.cpp | 135 ++++++++++++++++++++
WebKit/gtk/ChangeLog | 24 ++++
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp | 6 +-
WebKit/gtk/tests/testhttpbackend.c | 121 ++++++++++++++++++
WebKit/gtk/webkit/webkitdownload.cpp | 7 +-
WebKit/gtk/webkit/webkitnetworkrequest.cpp | 51 +++++++-
WebKit/gtk/webkit/webkitprivate.h | 13 ++-
WebKit/gtk/webkit/webkitwebframe.cpp | 4 +-
14 files changed, 443 insertions(+), 39 deletions(-)
Comment on attachment 30763 [details]
network request
Ok. r=me.
Comment on attachment 30763 [details] network request Landed as r44263; I had to go back on the auto_ptr->PassOwnPtr conversion, though, since the adopt method cannot take a PassOwnPtr, and I didn't think this patch should also change that, hope that is OK. Comment on attachment 30763 [details]
network request
Clearing r+ flag so it won't appear in the commit queue.
I'm closing this bug, and will open a new one for the API proposals. |