<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Frontend should have access to Resource Timing information"
href="https://bugs.webkit.org/show_bug.cgi?id=160095">bug 160095</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #284369 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Frontend should have access to Resource Timing information"
href="https://bugs.webkit.org/show_bug.cgi?id=160095#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Frontend should have access to Resource Timing information"
href="https://bugs.webkit.org/show_bug.cgi?id=160095">bug 160095</a>
from <span class="vcard"><a class="email" href="mailto:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284369&action=diff" name="attach_284369" title="WIP Patch - Timing info">attachment 284369</a> <a href="attachment.cgi?id=284369&action=edit" title="WIP Patch - Timing info">[details]</a></span>
WIP Patch - Timing info
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284369&action=review">https://bugs.webkit.org/attachment.cgi?id=284369&action=review</a>
Inspector parts and test look good to me. I'd like to see another patch addressing the comments, and then get some kind of nod from reviewers more experienced with networking. So r- while waiting for an updated patch.
<span class="quote">> Source/WebCore/loader/ResourceLoader.cpp:353
> + m_resourceStartTime = monotonicallyIncreasingTime();</span >
Seems this could be moved up a bit. There are two failure reasons above (content extensions, null request) that we may want to include in resource timing data.
Further, ResourceLoader::init, seems like it might be best, given that also happens before other internal cancels, or even applicationCache checks (which are more timestamps we would eventually want to add for resource timing).
<span class="quote">> Source/WebCore/loader/ResourceLoader.cpp:464
> + m_resourceEndTime = monotonicallyIncreasingTime();</span >
Again, there are two failures above that could bail before setting this end time. It might be better to move this up.
<span class="quote">> Source/WebCore/loader/ResourceLoader.h:232
> + double m_resourceStartTime;
> + double m_resourceEndTime;</span >
You should initialize these to some reasonable value. Such as 0.
double m_resourceStartTime { 0 };
Currently, because they are not initialized in the constructor, they are "uninitialized" and hold random values until assigned.
<span class="quote">> Source/WebCore/platform/network/ResourceLoadTiming.h:97
> // These are millisecond deltas from the navigation start.</span >
This comment is wrong. They are not from navigation start, they are from the resource's start time.
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/Resource.js:62
> + this._timing = timing || {};</span >
Same comment here as below.
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/Resource.js:476
> + this._timing = timing || {};</span >
We actually may want to be careful here. We were sending this timing data in the past (iOS 7-9 backends), but it was wrong. So we probably want to do something like this:
this._timing = timing || {};
// COMPATIBILITY (iOS 10): Resource Timing data was incomplete and incorrect. Do not use it.
// iOS 7 sent a requestTime and iOS 8-9.3 sent a navigationStart time.
if (typeof this._timing.requestTime === "number" || typeof this._timing.navigationStart === "number")
this._timing = {};
Or, because it was incomplete/inaccurate we might even want to just remove it from legacy backend protocols:
Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json
Source/WebInspectorUI/Versions/Inspector-iOS-8.0.json
Source/WebInspectorUI/Versions/Inspector-iOS-9.0.json
Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json
Either way, we want to protect against incorrect backend data.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>