[Webkit-unassigned] [Bug 40222] Use the new SoupURILoader API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 1 07:28:38 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=40222
--- Comment #10 from Sergio Villar Senin <svillar at igalia.com> 2010-07-01 07:28:38 PST ---
(In reply to comment #9)
> (From update of attachment 59635 [details])
> >- GFile* m_gfile;
> >+ SoupRequest* m_req;
>
> Maybe we can start using GRefPtr for this?
Seems a good place indeed
> >- String url = handle->request().url().string();
> >- ASSERT(url.startsWith("data:", false));
> >+ String urlStr = handle->request().url().string();
> >+ ASSERT(urlStr.startsWith("data:", false));
> >
> >- int index = url.find(',');
> >+ int index = urlStr.find(',');
> > if (index == -1) {
> > client->cannotShowURL(handle);
> > return false;
> > }
> >
> >- String mediaType = url.substring(5, index - 5);
> >- String data = url.substring(index + 1);
> >+ String mediaType = urlStr.substring(5, index - 5);
>
> This seems to be a bit random? Should go in a different patch at the very least.
Absolutely, I guess I changed it sometime in the past when there was another url variable around.
> >+ SoupSession* session = handle->defaultSession();
> >+ GError* error = 0;
>
> Should use GOwnPtr for GError here.
Ok
> >+ d->m_req = soup_session_request(session, handle->request().url().string().utf8().data(), &error);
> >+ if (error) {
> >+ g_error_free(error);
> >+ return false;
> >+ }
> >+
> >+ GInputStream* in = soup_request_send(d->m_req, 0, &error);
> >+ if (error) {
> >+ g_error_free(error);
> >+ return false;
> >+ }
> >+
> >+ d->m_inputStream = in;
> >+ d->m_bufferSize = 8192;
> >+ d->m_buffer = static_cast<char*>(g_malloc(d->m_bufferSize));
> >+ d->m_total = 0;
> >+
> >+ g_object_set_data(G_OBJECT(d->m_inputStream), "webkit-resource", handle);
> >+ handle->ref();
> >+
> >+ d->m_cancellable = g_cancellable_new();
> >+ g_input_stream_read_async(d->m_inputStream, d->m_buffer, d->m_bufferSize,
> >+ G_PRIORITY_DEFAULT, d->m_cancellable,
> >+ readCallback, (isBase64) ? 0 : GINT_TO_POINTER(1));
>
> I suppose it's good to unify stuff and always go through libsoup for everything, but isn't parsing a simple string asynchronously a bit overkill? /me wonders
I agree that the real benefit for the data: case is only to use a single path for every protocol, as what we do basically is to parse the string as you say. Don't feel like strongly defending any alternative though.
> >
> > return false;
> > }
> >@@ -428,7 +404,7 @@ static bool startData(ResourceHandle* handle, String urlString)
> >
> > // If parseDataUrl is called synchronously the job is not yet effectively started
> > // and webkit won't never know that the data has been parsed even didFinishLoading is called.
> >- d->m_idleHandler = g_timeout_add(0, parseDataUrl, handle);
> >+ d->m_idleHandler = g_idle_add(parseDataUrl, handle);
>
> This is not the same thing. Do you need to change it or you just felt it was better? Needs an explanation.
I agree, I have to find one first :-). Thing is that somehow tests show some flackiness with the timeout_add while they worked fine everytime for me with the idle stuff.
> > return true;
> > }
> >
> >@@ -469,6 +445,151 @@ static void ensureSessionIsInitialized(SoupSession* session)
> > g_object_set_data(G_OBJECT(session), "webkit-init", reinterpret_cast<void*>(0xdeadbeef));
> > }
> >
>
> >+static void sendRequestCallback(GObject* source, GAsyncResult* res, gpointer userData)
> >+{
> >+ RefPtr<ResourceHandle> handle = static_cast<ResourceHandle*>(g_object_get_data(source, "webkit-resource"));
> >+ if (!handle)
> >+ return;
> >+
> >+ ResourceHandleInternal* d = handle->getInternal();
> >+ ResourceHandleClient* client = handle->client();
> >+
> >+ if (userData) {
> >+ // No need to call gotChunkHandler anymore. Received data will
> >+ // be reported by readCallback
> >+ gulong gotChunkHandler = GPOINTER_TO_UINT(userData);
> >+ if (g_signal_handler_is_connected(d->m_msg, gotChunkHandler))
> >+ g_signal_handler_disconnect(d->m_msg, gotChunkHandler);
> >+ }
> >+
> >+ if (d->m_cancelled || !client) {
> >+ cleanupSoupRequestOperation(handle.get());
> >+ return;
> >+ }
> >+
> >+ GError* error = 0;
>
> GOwnPtr. This is repeated elsewhere, the same applies.
Yep
> >+ GInputStream* in = soup_request_send_finish(d->m_req, res, &error);
> >+
> >+ if (error) {
> >+
>
> Extra blank line!
>
> >+ if (d->m_msg && SOUP_STATUS_IS_TRANSPORT_ERROR(d->m_msg->status_code)) {
> >+ SoupURI* uri = soup_request_get_uri(d->m_req);
> >+ char* uristr = soup_uri_to_string(uri, false);
>
> uriStr? With GOwnPtr. This applies in a bunch of places too.
Fair enough
> >+ ResourceError resourceError(g_quark_to_string(SOUP_HTTP_ERROR),
> >+ d->m_msg->status_code,
> >+ uristr,
> >+ String::fromUTF8(d->m_msg->reason_phrase));
> >+ g_free(uristr);
> >+ g_error_free(error);
> >+ cleanupSoupRequestOperation(handle.get());
> >+ client->didFail(handle.get(), resourceError);
> >+ return;
> >+ }
> >+
> >+ if (error->domain == G_IO_ERROR) {
> >+ SoupURI* uri = soup_request_get_uri(d->m_req);
> >+ char* uristr = soup_uri_to_string(uri, false);
> >+ ResourceError resourceError(g_quark_to_string(G_IO_ERROR),
> >+ error->code,
> >+ uristr,
> >+ error ? String::fromUTF8(error->message) : String());
> >+ g_free(uristr);
> >+ g_error_free(error);
> >+ cleanupSoupRequestOperation(handle.get());
> >+ client->didFail(handle.get(), resourceError);
> >+ return;
> >+ }
> >+
> >+ g_error_free(error);
> >+
> >+ if (d->m_msg && d->m_msg->status_code == SOUP_STATUS_UNAUTHORIZED) {
> >+ fillResponseFromMessage(d->m_msg, &d->m_response);
> >+ client->didReceiveResponse(handle.get(), d->m_response);
> >+
> >+ // WebCore might have cancelled the job in the while
> >+ if (d->m_cancelled)
> >+ return;
> >+
> >+ if (d->m_msg->response_body->data)
> >+ client->didReceiveData(handle.get(), d->m_msg->response_body->data, d->m_msg->response_body->length, true);
> >+ }
> >+
> >+ client->didFinishLoading(handle.get());
> >+ return;
> >+ }
> >+
> >+ if (d->m_cancelled) {
> >+ cleanupSoupRequestOperation(handle.get());
> >+ return;
> >+ }
> >+
> >+ d->m_inputStream = in;
> >+ d->m_bufferSize = 8192;
>
> This number appears around more than once, should be a #define
Ok
> >+ d->m_buffer = static_cast<char*>(g_malloc(d->m_bufferSize));
> >+ d->m_total = 0;
> >+
> >+ // readCallback needs it
> >+ g_object_set_data(G_OBJECT(d->m_inputStream), "webkit-resource", handle.get());
> >+
> >+ // We need to check if it's a file: URL and if it is a regular
> >+ // file as it could be a directory. In that case Soup properly
> >+ // returns a stream whose outcome is a HTML with a list of files
> >+ // in the directory
> >+ if (equalIgnoringCase(handle->request().url().protocol(), "file")
> >+ && G_IS_FILE_INPUT_STREAM(in)) {
> >+
> >+ GFile* file = soup_request_file_get_file(SOUP_REQUEST_FILE(d->m_req));
>
> GRefPtr.
Sure
> >+
> >+ ResourceResponse response;
> >+
> >+ response.setURL(handle->request().url());
> >+ response.setMimeType(soup_request_get_content_type(d->m_req));
> >+ response.setExpectedContentLength(soup_request_get_content_length(d->m_req));
> >+ client->didReceiveResponse(handle.get(), response);
> >+
> >+ if (d->m_cancelled) {
> >+ cleanupSoupRequestOperation(handle.get());
> >+ return;
> >+ }
> >+
> >+ g_object_unref(file);
> >+ }
> >+
> >+ g_input_stream_read_async(d->m_inputStream, d->m_buffer, d->m_bufferSize,
> >+ G_PRIORITY_DEFAULT, d->m_cancellable,
> >+ readCallback, 0);
> >+}
> >+
>
> >
> >+
>
> Not extra line.
>
> > bool ResourceHandle::start(Frame* frame)
> > {
> > ASSERT(!d->m_msg);
> >@@ -591,21 +720,21 @@ bool ResourceHandle::start(Frame* frame)
> > return false;
> >
> > KURL url = request().url();
> >- String urlString = url.string();
> >- String protocol = url.protocol();
> >
> > // Used to set the authentication dialog toplevel; may be NULL
> > d->m_frame = frame;
> >
> >- if (equalIgnoringCase(protocol, "data"))
> >- return startData(this, urlString);
> >+ if (url.protocolIs("data"))
> >+ return startData(this, url);
> >
> >- if (equalIgnoringCase(protocol, "http") || equalIgnoringCase(protocol, "https")) {
> >+ if (url.protocolInHTTPFamily()) {
> > if (startHttp(this))
> > return true;
> > }
> >
> >- if (equalIgnoringCase(protocol, "file") || equalIgnoringCase(protocol, "ftp") || equalIgnoringCase(protocol, "ftps")) {
> >+ if (url.protocolIs("file")
> >+ || url.protocolIs("ftp")
> >+ || url.protocolIs("ftps")) {
> > // FIXME: should we be doing any other protocols here?
> > if (startGio(this, url))
> > return true;
>
> This is all nice but I don't believe it's strictly needed in the patch, you could push it separately and earlier.
>
That's true
> >@@ -665,38 +794,6 @@ void ResourceHandle::loadResourceSynchronously(const ResourceRequest& request, S
> > syncLoader.run();
> > }
> >
>
> > static void closeCallback(GObject* source, GAsyncResult* res, gpointer)
> > {
> > RefPtr<ResourceHandle> handle = static_cast<ResourceHandle*>(g_object_get_data(source, "webkit-resource"));
> >@@ -707,7 +804,6 @@ static void closeCallback(GObject* source, GAsyncResult* res, gpointer)
> > ResourceHandleClient* client = handle->client();
> >
> > g_input_stream_close_finish(d->m_inputStream, res, 0);
> >- cleanupGioOperation(handle.get());
> >
> > // The load may have been cancelled, the client may have been
> > // destroyed already. In such cases calling didFinishLoading is a
> >@@ -716,9 +812,11 @@ static void closeCallback(GObject* source, GAsyncResult* res, gpointer)
> > return;
> >
> > client->didFinishLoading(handle.get());
> >+
> >+ cleanupSoupRequestOperation(handle.get());
>
> Any reason to move this?
I just moved it because I thought it was releasing something too early, but it did not. Agree that it can be moved back to the middle of the function.
>
> > }
> >
>
> >@@ -755,141 +854,36 @@ static void readCallback(GObject* source, GAsyncResult* res, gpointer)
> > }
> >
> > d->m_total += bytesRead;
> >- client->didReceiveData(handle.get(), d->m_buffer, bytesRead, d->m_total);
> >+ if (G_LIKELY(!data))
> >+ client->didReceiveData(handle.get(), d->m_buffer, bytesRead, d->m_total);
>
> So the likely case is that there's no data read? Uh?
Maybe the name mislead you. That data is just a gpointer that I used as a flag in order to know whether we should convert the read bytes to UTF-16 or not. The most common case is that we don't have to, as it should be done only for non-bas64 data streams, which I guess are not so frequent as http: or another protocols.
>
> >+ else {
> >+ // We have to convert it to UTF-16 due to limitations in KURL
> >+ GOwnPtr<gunichar2> unicodeStr(g_utf8_to_utf16(d->m_buffer, bytesRead, 0, 0, 0));
> >+ client->didReceiveData(handle.get(), reinterpret_cast<const char*>(unicodeStr.get()), g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar), 0);
>
> Mmm, we don't seem to do this in the other didReceiveData calls, we pass UTF8 data and lenght in bytes, not characters. If the tests pass I suppose this is ok, but it seems puzzling.
> >+ }
> >
We're passing here also the length in bytes (that's why we have the sizeof(UChar). Anyway it's true that this is the only place where we do this. If you take a look at the original parseDataUrl method this is only done also for non-base64 data streams. And indeed, tests like it :)
Thx for reviewing!!!
--
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