[webkit-reviews] review granted: [Bug 42432] [GTK] Enable Web Timing : [Attachment 134524] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 42432: [GTK] Enable Web Timing
https://bugs.webkit.org/show_bug.cgi?id=42432

Attachment 134524: Patch
https://bugs.webkit.org/attachment.cgi?id=134524&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134524&action=review


LGTM

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:140
> +static int  currentTimeMinusRequestTime(double requestTime);

Can we give this function a more descriptive name, like
milisecondsSinceRequest?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:459
> +static void wroteBodyCallback(SoupMessage *, gpointer data)

* position

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:472
> +static void requestStartedCallback(SoupSession *, SoupMessage *soupMessage,
SoupSocket *, gpointer data)

The * is in the wrong side for all of them here.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:487
> +    if (d->m_response.resourceLoadTiming()->sslStart != -1)
> +	   // WebCore/inspector/front-end/ResourceTimingView.js assumes that
SSL time is included
> +	   // in connection time so must substract here the SSL delta that will
be added later.
> +	   d->m_response.resourceLoadTiming()->sendStart -=
> +	       d->m_response.resourceLoadTiming()->sslEnd -
d->m_response.resourceLoadTiming()->sslStart;

Can we have {} around this? Multiple lines make it harder to make sure it's
only a single statement. This comment could be improved too, I understood it
after reading a couple of times, but it's not obvious that somewhere the time
spent in ssl will be added somewhere (pointing to where would be good =D).

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:490
> +static void networkEventCallback(SoupMessage *, GSocketClientEvent event,
GIOStream *, gpointer data)

More *

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:513
> +	   if (d->m_response.resourceLoadTiming()->dnsStart != -1)
> +	       // WebCore/inspector/front-end/ResourceTimingView.js assumes
that DNS time is included
> +	       // in connection time so must substract here the DNS delta that
will be added later.
> +	       d->m_response.resourceLoadTiming()->connectStart -=
> +		   d->m_response.resourceLoadTiming()->dnsEnd -
d->m_response.resourceLoadTiming()->dnsStart;

Ditto the comment about SSL above.