[webkit-reviews] review denied: [Bug 89244] Web Inspector: Add data length to resource events on timeline to keep track of the amount of data loaded and the total data length : [Attachment 148881] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Hanna <hanma at rim.com>'s
request for review:
Bug 89244: Web Inspector: Add data length to resource events on timeline to
keep track of the amount of data loaded and the total data length
https://bugs.webkit.org/show_bug.cgi?id=89244

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=148881&action=review


> Source/WebCore/inspector/front-end/TimelinePanel.js:799
> +	       var contentLength =
this._presentationModel._contentLengthByRequestId[record.data["requestId"]];

You should not access private fields defined in other files.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:196
> +	   this._networkLoadedDataLength = {};

I don't think we really need all these details in the Timeline.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:197
> +	   this._contentLengthByRequestId = {};

ditto

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:537
> +		   if (this.data["length"])

I would leave only these two lines that are providing the actual instrumented
data.

> LayoutTests/inspector/timeline/timeline-network-received-data-expected.txt:6
> +ResourceReceivedData Properties:

This test might get flaky: what if there are multiple received data entries...
You should check that there is at least one received data entry and that it has
length instead.