[Webkit-unassigned] [Bug 161314] Web Inspector: Add resource timing model with timing information

[bugzilla-daemon at webkit.org]

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

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #287280|review?                     |review+
              Flags|                            |

--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 287280
  --> https://bugs.webkit.org/attachment.cgi?id=287280
Patch

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

r=me, though please double check with earlier iOS devices to make sure things still work there.

> Source/WebInspectorUI/ChangeLog:31
> +        Update timing object with info on from response.

Typo: "on from response"

> Source/WebInspectorUI/UserInterface/Main.html:355
> +    <script src="Models/ResourceTimingData.js"></script>

Style: Alphabetical order. Even though "Resource" uses "ResourceTimingData" the order is fine because neither are actually used until after all scripts have loaded.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:475
>          this._responseHeadersSize = String(this._statusCode).length + this._statusText.length + 12; // Extra length is for "HTTP/1.1 ", " ", and "\r\n".

Gosh, just noticed this wouldn't work for HTTP/2. Nothing you need to change.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:579
> +        this._timingData._responseEnd = elapsedTime || NaN;

Setting a private member like this is sketchy. Normally we would create some public method on ResourceTimingData and call here it. Something like:

    markResponseEndTime(responseEnd)

> Source/WebInspectorUI/UserInterface/Models/ResourceTimelineRecord.js:55
> -        return this._resource.requestSentTimestamp;
> +        return this._resource.timingData.startTime;

Same thing here.

> Source/WebInspectorUI/UserInterface/Models/ResourceTimingData.js:68
> +    // Static

Style: Static methods, like this, normally go above the Public block.

> LayoutTests/http/tests/inspector/network/resource-timing.html:11
> +function resolve(url) {
> +    let a = document.createElement('a');
> +    a.href = url;
> +    return a.href;
> +}

This function doesn't appear to be used. Drop it?

> LayoutTests/http/tests/inspector/network/resource-timing.html:13
> +function createRequest()
> +{

Nit: My style in tests would be to put this brace on the same line as the function name because it is outside of the test() function. But I may be the only one that uses this style.

> LayoutTests/http/tests/inspector/network/resource-timing.html:21
> +    let suite = InspectorTest.createAsyncSuite("ResourceTiming");

Lets name this after the model object "ResourceTimingData".

-- 
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/20160829/02dc97b6/attachment.html>