[Webkit-unassigned] [Bug 42432] Enable Web Timing for GTK

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Zan Dobersek <zandobersek at gmail.com>  2011-04-15 10:15:58 PST ---
(From update of attachment 84869)
View in context: https://bugs.webkit.org/attachment.cgi?id=84869&action=review

Thanks for the review!

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:63
>> +using namespace WTF;
> 
> Do we really need this?

True, this is not really needed. Will remove it and use WTF::currentTime().

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:412
>> +                                         0, 0, 0, 0, handle);
> 
> We do this in cleanupSoupRequestOperation, why would we need this here?

This disconnects every handler that is connected to the SoupSession object, more specifically it disconnects handlers to the connection-created and request-started signals. I don't see this already being done in the current code.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:573
>> +}
> 
> This seems correct but the problem is that you are handling the "connection-created" signal of any request issued to the SoupSession. Take into account that this callback will be called whenever the SoupSession needs to create a connection (and not only for this particular ResourceHandle). There are some other problems too, see below in the g_signal_connect() part

Is there a way of ensuring that the connection that was created definitely belongs to a certain ResourceHandle? Current attempt at this is that this callback is connected to the SoupSession object in either startHTTPRequest or startNonHTTPRequest and then disconnected in cleanupSoupRequestOperation, but is that sufficient enough?

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:587
>> +}
> 
> Same problem than above, the difference is that in this case you are able to check that the SoupMessage provided by libsoup in the second argument is the same than the one you have in d->m_soupMessage. Otherwise you'll have to discard it as it will be handled by another ResourceHandle.

I'll add that check.

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:646
>> +    g_signal_connect(session, "connection-created", G_CALLBACK(connectionCreatedCallback), handle);
> 
> This is not going to work for one reason. libsoup reuses the connections that have not been closed by the server whenever possible. In such cases libsoup just uses the connections, so no new connection is created and thus, the "connection-created" signal won't be emitted.

This might not be a problem as PerformanceTiming::connectStart returns previous time values when no connection (as in network request) is made and therefor connectStart variable is not set.

This part is located here: [1] http://trac.webkit.org/browser/trunk/Source/WebCore/page/PerformanceTiming.cpp#L200

>> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:879
>> +    g_signal_connect(session, "connection-created", G_CALLBACK(connectionCreatedCallback), handle);
> 
> Same comment than in the other "conneciton-created" signal connection and something more. Non-HTTP protocols may not involve any kind of network connection at all, for example data: or file:. For those protocols the "connection-created" signal does not make any sense as no connections are used.

In that case no connection should be made to this signal, will remove this statement.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.