[Webkit-unassigned] [Bug 160095] Web Inspector: Frontend should have access to Resource Timing information

[bugzilla-daemon at webkit.org]

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

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #284369|review?                     |review-
              Flags|                            |

--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 284369
  --> https://bugs.webkit.org/attachment.cgi?id=284369
WIP Patch - Timing info

View in context: https://bugs.webkit.org/attachment.cgi?id=284369&action=review

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.

> Source/WebCore/loader/ResourceLoader.cpp:353
> +    m_resourceStartTime = monotonicallyIncreasingTime();

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).

> Source/WebCore/loader/ResourceLoader.cpp:464
> +    m_resourceEndTime = monotonicallyIncreasingTime();

Again, there are two failures above that could bail before setting this end time. It might be better to move this up.

> Source/WebCore/loader/ResourceLoader.h:232
> +    double m_resourceStartTime;
> +    double m_resourceEndTime;

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.

> Source/WebCore/platform/network/ResourceLoadTiming.h:97
>      // These are millisecond deltas from the navigation start.

This comment is wrong. They are not from navigation start, they are from the resource's start time.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:62
> +        this._timing = timing || {};

Same comment here as below.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:476
> +        this._timing = timing || {};

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160727/d73a8d6f/attachment-0001.html>